Skip to content

Update FieldValidationInstrumentation.java #3066

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.

Sign up for GitHub

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

Jump to bottom
Merged
dondonz merged 2 commits into from
Jan 12, 2023
Merged

Update FieldValidationInstrumentation.java #3066

dondonz merged 2 commits into from
Jan 12, 2023

Conversation

@kfwerf
Copy link
Contributor

@kfwerf kfwerf commented Jan 3, 2023

Fix FieldValidationInstrumentation to forward the state

Fixes #3065

@kfwerf
Update FieldValidationInstrumentation.java
383f2cb 
Fix FieldValidationInstrumentation to forward the `state`
@kfwerf kfwerf mentioned this pull request Jan 3, 2023
Closed
@dondonz
Copy link
Member

dondonz commented Jan 4, 2023

Thanks! Could you also add a test that would have failed with the old behaviour, and would be fixed with your change?

@kfwerf
Add test
d6aef27 
Adds test for chainedInstrumentation which exerts different e2e behaviour
@kfwerf
Copy link
Contributor Author

kfwerf commented Jan 5, 2023

Thanks! Could you also add a test that would have failed with the old behaviour, and would be fixed with your change?

Done! Added one for ChainedInstrumentation

@kfwerf
Copy link
Contributor Author

kfwerf commented Jan 11, 2023

@dondonz any thoughts on this PR? We are blocked with upgrading until this gets resolved

@dondonz dondonz added this to the 21.0 milestone Jan 12, 2023
dondonz
dondonz approved these changes Jan 12, 2023
View reviewed changes
Copy link
Member

@dondonz dondonz left a comment

Choose a reason for hiding this comment

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

Thanks for finding the bug & writing the fix!

@dondonz dondonz merged commit e44b854 into graphql-java:master Jan 12, 2023
@dondonz
Copy link
Member

dondonz commented Jan 12, 2023

The master build version containing this fix will be available in Maven in a few minutes. Keep in mind one change between v20 and current master was the change from Java 8 to Java 11.

FYI we usually bundle up a few PRs together for a small release, so this fix will be included in the next release. In the meantime, you can use the master build version to get unblocked now.

@kfwerf
Copy link
Contributor Author

kfwerf commented Jan 12, 2023

cool, thank you @dondonz ! Appreciate the quickness on all of this :)

@andimarek andimarek modified the milestones: 21.0, April 2023 Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dondonz dondonz dondonz approved these changes

+2 more reviewers

@Ekryd Ekryd Ekryd approved these changes

@Aynemayne Aynemayne Aynemayne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

20.1

Development

Successfully merging this pull request may close these issues.

FieldValidationInstrumentation.beginExecuteOperation does not forward the state and throws assertionerror

5 participants

@kfwerf @dondonz @Ekryd @Aynemayne @andimarek