OrderingTerm

[tevelee]

Hi PointFree!

(Since the structured queries repo is still private, I'm sharing my feedback here)

In light of your recent episode concluding the ORDER BY functionality and transitioning to WHERE clauses, I wanted to highlight a potential issue introduced in the latest update to the library that makes possible to produce invalid SQL syntax.

By defining asc and desc on the QueryExpression protocol

extension QueryExpression {
  func asc(nulls: NullOrder? = nil) -> some QueryExpression {
    …
  }
  func desc(nulls: NullOrder? = nil) -> some QueryExpression {
    …
  }
}

…it’s now possible to write expressions like Reminder.select { $0.title.asc() }, which generates invalid SQL. According to the documentation, ASC can only be applied to ordering-terms, not general exprs.

In my opinion, ordering term deserves its own distinct type/protocol in the library to ensure type safety so that asc and desc functions are not available in .select {}, but other general expressions like length are applicable in both .select {} and .order {}.

Have you considered implementing a validation layer — perhaps using a regular expression or a swift-parsing-based parser — to ensure that the generated SQL conforms to the expected grammar?

[stephencelis]

Hi @tevelee. The code we write in episodes is often simplified, as is the case here. In the current alpha of the library, it is not possible for that line to compile.

While we do try to minimize the generation of invalid SQL, it is not a goal of the library to 100% catch every single edge case, and while none come to mind at the moment, I'm sure there are ways to generate invalid SQL if you try hard enough 😄

Our main goal is to provide a system that reasonably guides an individual already familiar with SQL. A user familiar with SQL should ideally recognize when a builder such as the one above is not valid, even if it did compile.

[tevelee]

I gave the – now released – library a spin. It still appears in autocorrect because asc() is defined as an extension on QueryExpression.

Fortunately tho it does not compile (throws a cryptic error message Static method 'select' requires that '(@Sendable (NullOrdering?) -> some QueryExpression).QueryValue' conform to 'QueryRepresentable'), but I believe even autocompletion can be avoided if it were defined on a dedicated order-specific type and QueryFragmentBuilder would be specialized to return that type on the .order() function.

[mbrandonw]

Hi @tevelee, would you be interested in sketching out what it would look like to prevent asc() from showing up in autocomplete for select but not order?

[tevelee]

I can give it a try. Will open a PR when I'll have time to work on it.

[tevelee]

I had a few minutes this evening, so I came up with a proof of concept solution on my fork here.
Hopefully that helps resolving this issue with some added type safety if you think this is an area worth pursuing.

I'm fine with it appearing in autocorrect, as long as it won't build and produce invalid results, which it does not.

[stephencelis]

I'm fine with it appearing in autocorrect, as long as it won't build and produce invalid results, which it does not.

@tevelee Maybe I'm missing something but isn't this the case with the code on main today? Can you explain the differences on your branch for me to better understand? Maybe two code samples that behave differently before/after?