Skip to content

Improve setter resolution logic in BaseConfigurator#2825

Merged
timja merged 5 commits intojenkinsci:masterfrom
somiljain2006:Overloaded-setter-ambiguity-fix
Apr 13, 2026
Merged

Improve setter resolution logic in BaseConfigurator#2825
timja merged 5 commits intojenkinsci:masterfrom
somiljain2006:Overloaded-setter-ambiguity-fix

Conversation

@somiljain2006
Copy link
Copy Markdown
Contributor

@somiljain2006 somiljain2006 commented Apr 11, 2026

Refactor setter resolution logic in BaseConfigurator. Improve how setters are selected when multiple overloads exist.

Key improvements:

  • Prefer exact type matches over subclasses and superclasses
  • Correctly prioritise more specific types (e.g., avoid Object when better match exists)
  • Handle ambiguous setters deterministically to avoid JVM reflection order issues
  • Preserve full Java polymorphism support

Additional changes:

  • Filter and pass only compatible setters to resolution logic
  • Added unit tests covering:
    • Exact match resolution
    • Subclass vs superclass handling
    • Object fallback behavior
    • Ambiguous setter scenarios

Manual verification:

  • YAML configuration applied successfully
  • No regressions observed in tool/plugin configuration

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

@somiljain2006 somiljain2006 requested a review from a team as a code owner April 11, 2026 17:52
@somiljain2006 somiljain2006 marked this pull request as draft April 11, 2026 18:24
@somiljain2006 somiljain2006 marked this pull request as ready for review April 12, 2026 10:47
@somiljain2006
Copy link
Copy Markdown
Contributor Author

@timja Can you review this pr?

@timja timja added the feature A PR that adds a feature - used by Release Drafter label Apr 12, 2026
Comment thread plugin/src/test/java/io/jenkins/plugins/casc/BaseConfiguratorTest.java Outdated
Comment thread plugin/src/test/java/io/jenkins/plugins/casc/BaseConfiguratorTest.java Outdated
@timja timja requested a review from Copilot April 12, 2026 20:57
Copy link
Copy Markdown
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

The code seems fine.

The main change is a getter prefixed with is that is not a boolean / Boolean will now be ignored. should be ok.

and then there's some slight changes to do with sub-classes, in the new test the only cases that fail on master seem to be for ambiguous or ordering ones.

Happy enough to take this but there's two minor test improvements that are needed

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors BaseConfigurator#describe() to deterministically select the most appropriate setter when multiple overloaded setters exist, improving correctness across exact matches, inheritance hierarchies, and ambiguous overloads.

Changes:

  • Group setters by property, locate the corresponding getter, and resolve the “best” setter deterministically (avoiding JVM reflection order dependence).
  • Add array-handling improvements by converting Collection inputs to arrays when the selected setter expects an array.
  • Add unit tests covering exact match resolution, subtype/supertype selection, Object fallback, ambiguity handling, primitive/wrapper matching, and missing/invalid getter cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java Reworks setter discovery/selection and introduces deterministic overload resolution + collection→array conversion for array setters.
plugin/src/test/java/io/jenkins/plugins/casc/BaseConfiguratorTest.java Adds comprehensive tests for the new setter resolution behavior and edge cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java
Comment thread plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java
@timja
Copy link
Copy Markdown
Member

timja commented Apr 12, 2026

can you check copilots suggestions (including adding test cases if appropriate)

@somiljain2006 somiljain2006 requested a review from timja April 13, 2026 03:01
@timja timja merged commit eaed8d4 into jenkinsci:master Apr 13, 2026
17 checks passed
@somiljain2006 somiljain2006 deleted the Overloaded-setter-ambiguity-fix branch April 13, 2026 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature A PR that adds a feature - used by Release Drafter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants