Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor query logic to support querying of nested structures #979

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

jcrossley3
Copy link
Contributor

@jcrossley3 jcrossley3 commented Nov 6, 2024

We enhance the Filter and Sort structs to deal with more complex LHS expressions, e.g. "table.column"->>'key', instead of just column names.

Specifically, this enables the querying of JsonBinary and Json columns.

This is enables the querying of JSON columns via the LHS syntax of
`"table.column"->>'key'`

Signed-off-by: Jim Crossley <[email protected]>
This may be useful for other types of nested structures, too.

Signed-off-by: Jim Crossley <[email protected]>
@jcrossley3 jcrossley3 changed the title Carry Expr's instead of ColumnRef's to make queries more flexible Refactor query logic to support querying of nested structures Nov 7, 2024
@jcrossley3 jcrossley3 marked this pull request as ready for review November 7, 2024 01:05
})
.cloned()
pub(crate) fn for_field(&self, field: &str) -> Option<(Expr, ColumnDef)> {
fn name_match(tgt: &str) -> impl Fn(&&(ColumnRef, ColumnDef)) -> bool + use<'_> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctron the compiler told me to add + use<'_> in order to define a fn that returns a closure. I'm not familiar with that syntax, and neither the error message nor google was much help.

error[E0700]: hidden type for `impl for<'a, 'b> Fn(&'a &'b (ColumnRef, sea_orm::ColumnDef)) -> bool` captures lifetime that does not appear in bounds
   --> common/src/db/query/columns.rs:134:13
    |
133 |           fn name_match(tgt: &str) -> impl Fn(&&(ColumnRef, ColumnDef)) -> bool {
    |                              ----     ----------------------------------------- opaque type defined here
    |                              |
    |                              hidden type `{closure@common/src/db/query/columns.rs:134:13: 134:23}` captures the anonymous lifetime defined here
134 | /             |(col, _)| {
135 | |                 matches!(col,
136 | |                          ColumnRef::Column(name)
137 | |                          | ColumnRef::TableColumn(_, name)
138 | |                          | ColumnRef::SchemaTableColumn(_, _, name)
139 | |                          if name.to_string().eq_ignore_ascii_case(tgt))
140 | |             }
    | |_____________^
    |
help: add a `use<...>` bound to explicitly capture `'_`
    |
133 |         fn name_match(tgt: &str) -> impl Fn(&&(ColumnRef, ColumnDef)) -> bool + use<'_> {
    |                                                                               +++++++++

If the compiler knows how to fix it, why tell me about it?!?! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, I can omit the use<> around the '_ lifetime, which I discovered after clippy complained about this:

        fn name_match<'a>(tgt: &'a str) -> impl Fn(&&(ColumnRef, ColumnDef)) -> bool + 'a {

Still curious about the use<> syntax, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a new thing with 1.82.0: https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#precise-capturing-use-syntax

You can omit it, having the previous behavior. But now the language gives you another tool for more fine grained definition of what you had in the mind. The compiler trying to educate you about new stuff.

Copy link
Contributor Author

@jcrossley3 jcrossley3 Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew you'd know, thanks for the direct link, which answered my original question:

Note that in Rust 2024, the examples above will "just work" without needing use<..> syntax
(or any tricks). This is because in the new edition, opaque types will automatically capture 
all lifetime parameters in scope. This is a better default, and we've seen a lot of evidence 
about how this cleans up code. In Rust 2024, use<..> syntax will serve as an important way 
of opting-out of that default.

Signed-off-by: Jim Crossley <[email protected]>
@ctron
Copy link
Contributor

ctron commented Nov 7, 2024

I think this looks good in general. I am just a bit worried we're exposing the internal JSON structure to the user. Allowing the user to specify a path to the data inside a field might: a) make it super tricky refactoring things, b) create a security problem if the user can access data that the user's not entitled to

Copy link
Collaborator

@JimFuller-RedHat JimFuller-RedHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ctron ctron added this pull request to the merge queue Nov 7, 2024
Merged via the queue into trustification:main with commit b77ebb3 Nov 7, 2024
2 checks passed
@jcrossley3
Copy link
Contributor Author

jcrossley3 commented Nov 7, 2024

I am just a bit worried we're exposing the internal JSON structure to the user. Allowing the user to specify a path to the data inside a field might: a) make it super tricky refactoring things, b) create a security problem if the user can access data that the user's not entitled to

That's a good point about unfettered access, and I share your concern. I think I can mitigate that by providing a method on the optional Columns struct that configures which fields in the query can be used as keys in a particular json column. I'll work up a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants