♻️ Add macro for generating smart filters

[JMicheli]

This is an initial cut at adding a macro to generate smart filters. I still need to implement more robust error handling in the macro and split the crate up a bit more reasonably, but I wanted to make sure the API fit in with the way these filters are expected to develop.

It does a pretty good job cutting down on boilerplate so far.

[codecov]

Codecov Report

Attention: Patch coverage is 89.62963% with 28 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/smart-filter-gen/src/enum_data.rs	78.57%	12 Missing ⚠️
crates/smart-filter-gen/src/gen_match_arms.rs	93.18%	6 Missing ⚠️
crates/smart-filter-gen/src/lib.rs	94.89%	5 Missing ⚠️
crates/stump-config-gen/src/config_vars.rs	33.33%	4 Missing ⚠️
core/src/db/filter/smart_filter.rs	75.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
crates/stump-config-gen/src/gen_config_impls.rs	92.94% <100.00%> (ø)
crates/stump-config-gen/src/gen_partial_config.rs	98.87% <100.00%> (ø)
crates/stump-config-gen/src/type_utils.rs	88.13% <100.00%> (ø)
core/src/db/filter/smart_filter.rs	75.94% <75.00%> (ø)
crates/stump-config-gen/src/config_vars.rs	90.69% <33.33%> (ø)
crates/smart-filter-gen/src/lib.rs	94.89% <94.89%> (ø)
crates/smart-filter-gen/src/gen_match_arms.rs	93.18% <93.18%> (ø)
crates/smart-filter-gen/src/enum_data.rs	78.57% <78.57%> (ø)

[aaronleopold]

Hey! So I gave this a read-through and don't see any issues really. I am still not very knowledgeable with macros, but I was able to follow at least a bit of what you've done (thank you for the doc comments). One thing I'm curious about that I don't yet understand is how there aren't duplicated impl blocks, e.g. impl Filter<String> for each different filter enum (e.g. MediaSmartFilter, SeriesSmartFilter, etc).

Since it is hypothetically generating analogous code I wouldn't expect an issue, but I'm curious if you've spun up a server and tested this yet?

I wanted to make sure the API fit in with the way these filters are expected to develop

I don't think I see anything which conflicts with the direction of the feature, so in this sense I think it fits - I don't foresee any massively breaking changes to the feature at least. I know there is a comment somewhere I wrote detailing a scenario where you stack a bunch of relational filters which has performance implications, but that is so complex that it definitely won't be thought about until the far future.

One thing to call out is what I mentioned on Discord wrt supporting Filter<DateTime<FixedOffset>> (e.g. link). With my active feature branch containing the smart list UI updates, these are what I am aiming to be able to configure. I think this will be a good exercise to see how a new type should be added, if it would need to be done again down the road

Thank you for knocking this out! You mentioned there are still some things you're aiming to clean up so I won't give my approval yet. Feel free to re-request review and/or remove the draft status when you'd like me to look again

[JMicheli]

One thing to call out is what I mentioned on Discord wrt supporting Filter<DateTime<FixedOffset>> (e.g. link). With my active feature branch containing the smart list UI updates, these are what I am aiming to be able to configure. I think this will be a good exercise to see how a new type should be added, if it would need to be done again down the road

Responding to this portion of your comment first. I'm going to make an addition adding support for this, and it should hopefully demonstrate the process of updating the macro decently well. I'll also try to document it.

The way you'd go about updating this macro should be quite simple. First you'll add a new branch to the match arm generator function, generate_match_arm, that will match on the type you want to add handling for. That arm should call the function you will create in step 2 (just like the two others currently existing). In step 2, you copy generate_string_match_arm or whatever as a base, and define a new function that properly forms the match arm contents the way you'd like. Read the documentation for the quote crate if anything in that function is unclear.

Actually one other step, you need to also update should_filter_type and add the new type to the first match arm if it should be wrapped in Filter when the enum is generated. This will probably always be the case for new types.

[JMicheli]

Hey! So I gave this a read-through and don't see any issues really. I am still not very knowledgeable with macros, but I was able to follow at least a bit of what you've done (thank you for the doc comments). One thing I'm curious about that I don't yet understand is how there aren't duplicated impl blocks, e.g. impl Filter<String> for each different filter enum (e.g. MediaSmartFilter, SeriesSmartFilter, etc).

Since it is hypothetically generating analogous code I wouldn't expect an issue, but I'm curious if you've spun up a server and tested this yet?

Admittedly I haven't tested it yet, but I did confirm that it generates the same code I removed. I'll test it properly before I un-draft the PR.

I don't think I fully understand the scenario you describe, but I'll walk through how the macro works and see if that answers it.

The macro operates in two phases.

Phase 1

The macro takes in an enum definition (that includes the #[prisma_table = "whatever"] and #[is_optional] annotations) and peels it apart using the syn crate.

It takes apart the attributes on the outside of the enum and looks for prisma_table, taking its value if it is there and failing if it is not. It removes this from the attributes list so that it can reattach those to the final enum.

The rest of the enum becomes a chunk of data, DestructedEnum, consisting of a Vec of EnumVariant data. Each EnumVariant tracks:

1. the name of the enum variant (variant_name)
2. the name of the field inside that enum (variable_inner_name) - this must match the prisma column name being filtered
3. the type of that inner field (variable_type)
4. whether or not if found an #[is_optional] attribute

The rest of generation doesn't operate on tokens, it operates on that data.

Phase 2

The "real" enum is constructed by gen_smart_filter_enum. This just recreates the input enum, but with some modifications. For each field, it checks the field's type stored above using should_filter_type, and for any type in that function (i.e., a String, a number type) it will return true and wrap the type so that it's Filter<T>. If it's not a type listed, it assumes it must be a smart filter and uses it as is. It also slaps the attributes (minus prisma_table) back on the enum so it has all its derives and serde annotations.

Next, the into_params implementation is generated by gen_smart_filter_impls. This function loops over the variants and fills in match arms depending on the data stored in phase 1:

1. For String it will generate an arm with generate_string_match_arm
2. For numbers it uses generate_number_match_arm
3. For everything else (fallthrough in the match statement) it uses generate_filter_match_arm, and generates code that should fail to compile unless the type was a smart filter.

So I think the answer to your question, in summary, is that it doesn't generate redundant impls because impls only get generated for things when they are annotated with the macro. It doesn't recurse into the enum and generate anything for contained types.

[aaronleopold]

@JMicheli Your explanation / walk-through was absolutely perfect, thank you!

So I think the answer to your question, in summary, is that it doesn't generate redundant impls because impls only get generated for things when they are annotated with the macro. It doesn't recurse into the enum and generate anything for contained types.

Makes sense now 👍

[JMicheli]

Btw I updated it to handle DateTime<FixedOffset> it was a very simple change since those get handled the same way as numbers, so I just made that branch match on "DateTime" and it worked right off the bat.

I still need to make the error handling a little nicer and I am questioning whether or not the idea of just specifying, e.g., String in the input and then having the macro add the Filter<T> wrapping was a good call. It's less explicit and maybe confusing to read? Less keystrokes though.

[aaronleopold]

Btw I updated it to handle DateTime it was a very simple change since those get handled the same way as numbers, so I just made that branch match on "DateTime" and it worked right off the bat.

Nice! I see it's two lines, which is awesome.

I am questioning whether or not the idea of just specifying, e.g., String in the input and then having the macro add the Filter<T> wrapping was a good call

I think it's a good call, probably? Otherwise the Filter would materialize from 'nowhere' right? I know it literally wouldn't, it would be generated from the macro, but the context for where it came from might be unclear? I'm happy deferring to what you think is best there, I don't think I have a strong opinion besides the fact that I enjoy fewer keystrokes

[JMicheli]

Yeah the materialization from nowhere was my main concern. But it does simplify writing new smart filters so I think it's okay for now.

I'll change it if my assumption that this makes sense proves to be wrong.

Once I clean up error handling this should be good to merge. Would you prefer to merge it on top of your current filter changes or right into develop?

[aaronleopold]

Sounds good!

Would you prefer to merge it on top of your current filter changes or right into develop?

Let's rebase this PR to experimental and merge it there when ready. I'll rebase my feature branch once this is merged and fix any conflicts