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

feat: allow unchecked SQL operators #3698

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Dec 11, 2023

What this PR changes/adds

This PR adds an overloaded constructor for the SqlQueryStatement class that allows
to disable the operator checking.
That means, that the SqlQueryStatement won't throw an IllegalArgumentException anymore when it encounters
a supposedly illegal operator.

Why it does that

Postgres has some very specific operators, such as the ?, which can be used for JSONB (not JSON!) types. The ? can be used to comfortably check if a value occurs in a JSON array:

SELECT *
FROM sometable
WHERE (someJsonColumn -> 'stringarray_prop')::jsonb ? 'someval_that_occurs_in_the_array';

Check out this stackoverflow article for another example.

Now, since we do strict checks on =, in, like, constructing such a query would not be possible.

Further notes

  • It would have been possible to simply at the ? operator to the SqlConditionExpression.SUPPORTED_PREPARED_STATEMENT_OPERATORS list, but then we're dragging Postgres-specific operators into the otherwise generic SqlQueryExpression and that felt wrong.

    Although I haven't verified this, there could be other operators. Typically, if the usage of those operators is needed, we're dealing with a very niche use case, where operator validation can likely be foregone.

  • In passing, I added a branch in the the criterion converter to account for enum ordinals, not just literals. That appears to have been a bug.

  • Added an overloaded constructor for the ReflectionBasedQueryResolver to customize the criterion-to-predicate conversion

Linked Issue(s)

Needed for eclipse-edc/IdentityHub#188

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (07dfdcc) 71.73% compared to head (37f1157) 71.74%.

Files Patch % Lines
.../core/store/CriterionToPredicateConverterImpl.java 80.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3698   +/-   ##
=======================================
  Coverage   71.73%   71.74%           
=======================================
  Files         919      919           
  Lines       18452    18458    +6     
  Branches     1034     1037    +3     
=======================================
+ Hits        13236    13242    +6     
  Misses       4757     4757           
  Partials      459      459           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paullatzelsperger paullatzelsperger merged commit c6d378d into eclipse-edc:main Dec 12, 2023
20 of 23 checks passed
@paullatzelsperger paullatzelsperger deleted the feat/allow_unchecked_operators branch December 12, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core feature enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants