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

fix: make analytic expression visitor null-safe #1944

Conversation

davidjgoss
Copy link
Contributor

This PR adds some null safety via Optionals to the ExpressionVisitorAdapter when handling certain analytic expressions.

The added test expression previously parsed fine but resulted in a NullPointerException when visited.

@davidjgoss davidjgoss force-pushed the fix/analytic-window-element-nulls branch from 9dd206c to caf17a2 Compare January 10, 2024 11:07
@manticore-projects
Copy link
Contributor

Can we please conclude on this? I don't like stale PRs.

@davidjgoss
Copy link
Contributor Author

Sorry @manticore-projects - what is this PR missing?

@manticore-projects
Copy link
Contributor

Sorry @manticore-projects - what is this PR missing?

As explained before:

_Apologies for being difficult: Without the original expression, I have actually no idea what those code lines want or achieve (simply because I never understood or liked the streaming/functional API).

Since I can't stop you from using it (its valid after all), please keep at least the original code lines as comments so we have a clue what this is supposed to mean and to check.

Question: the Null test is done rather beautifully in Groovy and much easier to read.
Does anyone know a similar way avoiding the Streaming API?
_

@davidjgoss
Copy link
Contributor Author

Ah, I don't see that earlier comment for some reason.

I can't stop you from using it

I mean, you definitely can. It's not in keeping with the style in the rest of the project so I'm more than happy to change it to a more conventional null check, and add some commentary.

@manticore-projects
Copy link
Contributor

Stopping you is not in my interest and your code is correct and good.
Just add comments to support understanding as the syntax is rather complex and not self explaining (at least to me).

Thanks a lot!

@davidjgoss davidjgoss force-pushed the fix/analytic-window-element-nulls branch 2 times, most recently from ba41730 to 8e46e7a Compare February 14, 2024 12:50
@manticore-projects
Copy link
Contributor

Run './gradlew :spotlessApply' to fix these violations.

Copy link
Contributor

@manticore-projects manticore-projects left a comment

Choose a reason for hiding this comment

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

Thanks!

@davidjgoss davidjgoss force-pushed the fix/analytic-window-element-nulls branch from 8e46e7a to f23f59c Compare February 14, 2024 14:53
@davidjgoss
Copy link
Contributor Author

(fixed spotless, sorry about that)

@manticore-projects manticore-projects merged commit 768c63f into JSQLParser:master Feb 14, 2024
3 checks passed
@manticore-projects
Copy link
Contributor

Thank you for your contribution and effort!

@davidjgoss davidjgoss deleted the fix/analytic-window-element-nulls branch February 15, 2024 03:28
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