GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: make analytic expression visitor null-safe by davidjgoss · Pull Request #1944 · JSQLParser/JSqlParser

Conversation

@davidjgoss

@davidjgoss davidjgoss commented Jan 10, 2024

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jxnu-liguobin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@davidjgoss
davidjgoss force-pushed the fix/analytic-window-element-nulls branch from 9dd206c to caf17a2 Compare January 10, 2024 11:07
@manticore-projects

manticore-projects commented Feb 12, 2024

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@davidjgoss

davidjgoss commented Feb 12, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@manticore-projects

manticore-projects commented Feb 12, 2024

Copy link
Copy Markdown
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?
_

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@davidjgoss

davidjgoss commented Feb 13, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@manticore-projects

manticore-projects commented Feb 13, 2024

Copy link
Copy Markdown
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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

manticore-projects commented Feb 14, 2024

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

manticore-projects
manticore-projects approved these changes Feb 14, 2024
View reviewed changes

@manticore-projects manticore-projects left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/main/java/net/sf/jsqlparser/expression/ExpressionVisitorAdapter.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

@davidjgoss
fix: make analytic expression visitor null-safe
f23f59c
@davidjgoss
davidjgoss force-pushed the fix/analytic-window-element-nulls branch from 8e46e7a to f23f59c Compare February 14, 2024 14:53
@davidjgoss

davidjgoss commented Feb 14, 2024

Copy link
Copy Markdown
Contributor Author

(fixed spotless, sorry about that)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@manticore-projects
manticore-projects merged commit 768c63f into JSQLParser:master Feb 14, 2024
@manticore-projects

manticore-projects commented Feb 14, 2024

Copy link
Copy Markdown
Contributor

Thank you for your contribution and effort!

davidjgoss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

Reviewers

@manticore-projects manticore-projects manticore-projects approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@davidjgoss @manticore-projects
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.