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

[JENKINS-69899] Do not visit EmptyExpression when transforming fields declared using @Field #457

Merged
merged 4 commits into from
Oct 20, 2022

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Oct 19, 2022

See JENKINS-69899 and jenkinsci/groovy-sandbox#94.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue Tests are in groovy-sandbox

@@ -418,7 +418,7 @@ private interface Rejector {
Checker.preCheckedCast(field.getType(), value, false, false, false);
return super.onSetAttribute(invoker, receiver, attribute, value);
} else {
throw StaticWhitelist.rejectField(field);
rejector = () -> StaticWhitelist.rejectField(field);
Copy link
Member Author

@dwnusbaum dwnusbaum Oct 20, 2022

Choose a reason for hiding this comment

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

Not related to JENKINS-69899, but fixes a minor issue in 85d16bd. See #298 (comment) for details.

I am fixing it here in case we end up backporting the JENKINS-69899 fix, since it would be nice to backport this as well. That said, the cases it fixes were already broken before 85d16bd, so it is not particularly important.

Comment on lines +914 to +915
} else {
errors.addError(x);
Copy link
Member Author

Choose a reason for hiding this comment

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

This also fixes a minor test-only issue in 85d16bd. We would also want this backported to avoid introducing broken tests.

@dwnusbaum dwnusbaum marked this pull request as ready for review October 20, 2022 17:08
@@ -62,7 +62,7 @@
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>groovy-sandbox</artifactId>
<version>1.30</version>
<version>1.31</version>
Copy link
Member

Choose a reason for hiding this comment

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

BTW I think it would be a good investment of time to inline the lib into this plugin repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, enough of the pieces are in place now that it should be straightforward. I think the main issue would be checking to see if a new PCT hook is needed. I will try to do it as a Hacktoberfest project.

@dwnusbaum dwnusbaum added the bug label Oct 20, 2022
@dwnusbaum dwnusbaum merged commit bab7c8f into jenkinsci:master Oct 20, 2022
@dwnusbaum dwnusbaum deleted the JENKINS-69899 branch October 20, 2022 19:04
daniel-beck pushed a commit that referenced this pull request Nov 15, 2022
[JENKINS-69899] Do not visit EmptyExpression when transforming fields declared using @field

(cherry picked from commit bab7c8f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants