[Proposal] Fragments with named parameters

[Philipp 'Tessi' Tessenow]

Hey :) I want to make the proposal for named params in fragments.

Current Situation

Currently, fragments are written like `fragment("? + ?", 40, 2)` which is great and simple for small fragments with few distinct params. However, there are some problems:

• Big fragments have readability problems - one has to switch between reading the fragment string and the params list. This adds accidental cognitive load.
• If there are many fragment params, one has to manually keep the params in the correct order.
• If a param is used multiple times within the query, one has to list if just as often in the params list.

This is nicely demonstrated by a fragment I created for my $day_job recently:

fragment(

"? < ? OR (? = ? AND ? <= ?)",

service.invoice_year,

^max_year,

service.invoice_year,

^max_year,

service.invoice_month,

^max_month

)

Let's ignore the fact, that this could be solved in other ways (like creating dates and comparing those instead).

Just looking at the fragment string ("? < ? OR (? = ? AND ? <= ?)") is confusing and one has to go back and forth between it and the params to verify it works. Some params (e.g. ^max_year) are listed more than once in the params list and the params need to be correctly ordered to work with the 6 question marks in that query string. I don't look forward changing/debugging that little fragment when I have to touch it next :) The situation gets worse for actually big fragments.

Proposal

I want to make the proposal for named fragment query-strings.

fragment(

":service_year < :year OR (:service_year = :year AND :service_month <= :month)",

service_year: service.invoice_year,

service_month: service.invoice_month,

year: ^max_year,

month: ^max_month

)

• the query string gets longer, but way easier to read
• we can order our params however we want
• params don't need to be duplicated

My argument for named params is mostly around readability and maintainability of code - which, in my experience, is a pretty high priority in complex systems (since they tend to produce complex ecto queries). One could also make an argument around efficiency by arguing that duplicated params only need to be passed-in once.

Other Discussions

• similar proposal on this list from 2018 https://groups.google.com/g/elixir-ecto/c/QImgfh9h-t8/m/idpfxfVfAwAJ
• a discussion on Elixir forum from 08/2022 https://elixirforum.com/t/how-to-pass-named-or-numbered-parameters-to-ectos-fragment/49525?u=tessi

Implementation

Now, I'm no expert in Ecto internals, but I'm confident there is a (efficient) way to implement this proposal.

And I'm sure there is plenty opportunity to bike-shed around the syntax :) I chose a symbol-like syntax (:param_name) in my example to make its use in the fragment query similar looking to the keyword param. There is other possible ways to interpolate params (e.g. ?{param_name})  which might cause less collisions with SQL.

I went ahead and implemented a named_fragment() macro which implements a fragment with named params and expands into regular a fragment call. This might help us to play with the idea and validate it:

https://github.com/tessi/ecto_named_fragment

My implementation might solve readability and maintainability problems but can not be more efficient than fragment(), because after macro-expansion params are duplicated again.

cheers,
tessi

[José Valim]

The biggest concern is conflict with some eventual SQL notation. What if we allow interpolation directly in the string?

--
You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/4538f297-a51c-48b3-83e9-672e7552eba3n%40googlegroups.com.

[Philipp 'Tessi' Tessenow]

You mean like the following?

fragment("#{service.invoice_year} < #{^max_year} OR (#{service.invoice_year} = #{^max_year} AND #{service.invoice_month} <= #{^max_month})")

This would be even better regarding readability and maintainability. I can't imagine how #{^max_year} would work though. Maybe another interpolation syntax like ?{^max_year} that the Ecto macro-magic can take apart?

[José Valim]

Fragment is a macro so it can see inside the interpolation. Although I guess the reason we did not do this all this time is because we would be teaching people to interpolate into queries, which is not good.

We could pick another syntax, such as ?{} but then we don’t get syntax highlighting.

Another idea is to introduce ~FRAGMENT{…} now that we have uppercase letter sigils and allow interpolation in them.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/71b51782-e8ae-49e4-acb9-ce6e31613898n%40googlegroups.com.

[Christopher Keele]

A ~SQL{} sigil for fragments would be amazing! Especially if syntax highlighters were taught to treat the contents as SQL, sort of the way many do for regex sigils today.

~SQL{#{service.invoice_year} < #{^max_year} OR (#{service.invoice_year} = #{^max_year} AND #{service.invoice_month} <= #{^max_month})}

[José Valim]

Yeah, I am inclined to go down this route, but I would use ~FRAGMENT"...". Would you like to send a PR, Philipp?

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/7727d500-244b-4369-8bda-c044b64c8840n%40googlegroups.com.

[Philipp 'Tessi' Tessenow]

I'll have a stab, but I can't promise any timelines (kids, family and $day_job get priority :))

So far ecto internals look magic and scary to me. But I can only learn by doing it :)

How do you imagine this to be built? Probably similar to the current fragment() macro, right? Or do you imagine this to be something on top of fragment, that expands into a fragment() macro eventually (like the library i mentioned earlier is doing)?

[José Valim]

We have an Ecto.Query.Builder that traverses the AST.  You should find fragment AST in there (search for :fragment). You should then pattern match on `~FRAGMENT"..."` ast and convert it to the same internal representation as we do for fragment. If for some reason you want to delegate this to the Ecto team instead, feel free to open up an issue.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/75d34fa0-8c43-48e2-8c90-868ef3d8cc62n%40googlegroups.com.

[Christopher Keele]

Ah, I see—this wouldn't be a first-class sigil then, that could be used outside of the Query Builder macros? In that context, ~FRAGMENT does make perfect sense, and I see now how clever hoisting sigil AST to accomplish this is.

I interpreted this as something different, that I could make an argument for, but probably as a separate proposal: a first-class sigil like ~SQL that performed this same named interpolation into a string for usage in direct Ecto.Adapters.SQL.query/4 calls. As someone who uses a lot of PG-specific functionality I tend to write raw SQL DDLs rather than using a DSL in my migrations, but I have to interpolate Elixir into them reasonably often.

> Although I guess the reason we did not do this all this time is because we would be teaching people to interpolate into queries, which is not good.

😅 This is a good point, and I imagine I'd have to make a pretty strong argument to overcome it. I'll probably watch how this feature evolves and play around with a lib based on it, and experiment with some stronger guard rails for safer interpolation, first. Butting out of this proposal for now, but I really like it!

[Bernardo Amorim]

I was going to post that exact suggestion after I hacked some implementation around it (https://gist.github.com/bamorim/d250dc339bb5a07d7d2a2310b703d1d3)

Maybe this could be turned into a library or I can open a PR on Ecto itself?

Of course this probably needs more work to make it nicer, but this is a start.

[Bernardo Amorim]

phi..., I hope you don't mind but I opened a PR on Ecto today to start a conversation. I just did it because I had the code almost ready here and found about this thread afterwards.

https://github.com/elixir-ecto/ecto/pull/4239

[Philipp 'Tessi' Tessenow]

I don't mind at all! Thanks for starting this PR and all the discussion around it.

I did an attempt at making ~FRAGMENT work before I saw your post, learned a lot of how Ecto works and then saw your post/PR. And I'm not mad at all, just happy that you got further than I and I still learned something :) best possible outcome, so thanks!