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: Allow multiple EXTRACTJSONFIELD invocations on different paths (#8122) #8123

Conversation

stefanfrehse
Copy link
Contributor

@stefanfrehse stefanfrehse commented Sep 13, 2021

Description

Allows the extract function to be invoked on the same instance multiple times with different path variables.

Testing done

A unit test has been added and the steps that reproduce the bug validated.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@stefanfrehse stefanfrehse requested a review from a team as a code owner September 13, 2021 11:59
Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

It looks good.
+1

Comment on lines 50 to 51
final JsonPathTokenizer tokenizer = new JsonPathTokenizer(path);
final List<String> tokens = ImmutableList.copyOf(tokenizer);
Copy link
Member

Choose a reason for hiding this comment

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

It should be good to keep a the tokens initialized once in case the path is equals to the latest path. What about adding a new class variable to keep the latest path seen, and check if the new path is equals to the latest path, if true, then re-use the current tokens class variable, if not, then initialize it with a new JsonPathTokenizer and List objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks for the feedback. I will cover your points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@stefanfrehse stefanfrehse force-pushed the fix/allow-multiple-extractjsonfield-invocation-8122 branch from 323e1a9 to 105f448 Compare September 17, 2021 09:03
@spena
Copy link
Member

spena commented Sep 20, 2021

@stefanfrehse I created a PR for testing your PR here - #8152. There are some limitations for allowing testing external contributions. I'll merge this PR once the tests pass. If they do not pass, could you check what needs to be fixed?

@stefanfrehse
Copy link
Contributor Author

@stefanfrehse I created a PR for testing your PR here - #8152. There are some limitations for allowing testing external contributions. I'll merge this PR once the tests pass. If they do not pass, could you check what needs to be fixed?

It was running locally fine. Do I see as an external contributor why the build fails?

@spena
Copy link
Member

spena commented Sep 20, 2021

@stefanfrehse nvm, I see your job run, but failed. In the past, I've seen issue with contributors PRs not running. But this one just run, so you can take a look at the issue.

@stefanfrehse stefanfrehse force-pushed the fix/allow-multiple-extractjsonfield-invocation-8122 branch from 105f448 to 6980364 Compare September 20, 2021 19:26
@stefanfrehse
Copy link
Contributor Author

@spena build fixed. Thanks to the static analysis, bug has been detected and is fixed now.

@spena spena merged commit 7f1d407 into confluentinc:master Sep 21, 2021
@spena
Copy link
Member

spena commented Sep 21, 2021

I merged the PR. Thanks @stefanfrehse for your contribution.

@stefanfrehse stefanfrehse deleted the fix/allow-multiple-extractjsonfield-invocation-8122 branch September 23, 2021 15:57
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