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

Allow use of logical operators with single arg #224

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

adamruzicka
Copy link
Collaborator

Previously, searching for things like & thing constructed the AST incorrectly. Parsing & thing would produce the AST which would simplify into <AST::OperatorNode [:and, "thing"]>, which then couldn't really be used for searching.

This change builds the AST fully, meaning this will accept even things like & & & & & & thing which will then simplify into a single LeafNode.

Previously, searching for things like `& thing` constructed the AST
incorrectly. Parsing `& thing` would produce the AST which would simplify into
`<AST::OperatorNode [:and, "thing"]>`, which then couldn't really be used for searching.

This change builds the AST fully, meaning this will accept even things like
`& & & & & & thing` which will then simplify into a single LeafNode.
@ofedoren
Copy link

Thanks, @adamruzicka, seems to work fine with most (if not all) corner cases, but I think I've found one more:

If I'm not mistaken, then if the whole string starts with ^, it then fails on a "garbage" string, e.g.:

  • #$% - works fine
  • ^#$% - doesn't work
  • #@)%!#*^!*#)$^*!#$%!#$&!%$&*%^* - works fine
  • ^#@)%!#*^!*#)$^*!#$%!#$&!%$&*%^* - doesn't work (the first character is ^)

@adamruzicka
Copy link
Collaborator Author

Good catch, ^ is the IN operator which expects to have a left hand side. We could:

  • support lhs-less IN (and NOTIN), turning ^ (a,b,c) into a OR b OR c
  • not support it, fail early and report it as a query syntax error

In this case, I'd probably go with the second option. Opinions?

@ofedoren
Copy link

In this case, I'd probably go with the second option. Opinions?

That could do it, but then again, we do support "weird stuff" and don't throw a syntax error, but silently "support" it. Wouldn't it be inconsistent in that case?

@adamruzicka
Copy link
Collaborator Author

Right, went with 1 for the sake of consistency

@ofedoren
Copy link

ofedoren commented Oct 23, 2024

Some more chaotic testing:

!&(*%($) input throws Error: Value expected but found :lparen

^)(&#$%_@)#$)^*&$_#*^&_@!_ input throws Error: Empty list of operands

!)@#&%$*) inputs throws Error: external method 'find_by_activation_key' failed with error: PG::InvalidSchemaName: ERROR: schema "katello_activation_keys" does not exist LINE 1: ...activation_key_id" WHERE "hosts"."type" = $1 AND (katello_ac... ^
If the first two seems fine, it's obviously a syntax error (although we decided to ignore those?), the last one is more an internal issue, so is a valid "corner-case". I'm afraid my testing is not ideal, and even if we continue I might find some more cases. Now I'd rather think what should we do with those unrealistic inputs. Should we even bother with them or simply fix original issue?

@adamruzicka
Copy link
Collaborator Author

Should we even bother with them or simply fix original issue?

I'd say the original issue we set out to resolve was resolved. We resolved the things which sort of make sense and made left-hand-side-less handling a little bit more consistent. There are still cases in which the input is just unparseable, if that's even a word, and in that case halting with an error feels acceptable.

Copy link

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @adamruzicka, fine by me then :)

@adamruzicka adamruzicka merged commit f9fab94 into wvanbergen:master Oct 31, 2024
10 of 18 checks passed
@adamruzicka adamruzicka deleted the unary-infix-logical branch October 31, 2024 12:02
@adamruzicka
Copy link
Collaborator Author

Thank you @ofedoren !

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.

2 participants