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 the non-default constructor mechanism of bytecode recording [2.13] #31212

Closed
wants to merge 4 commits into from

Conversation

gastaldi
Copy link
Contributor

Bytecode recorders attempt to match constructor parameters to properties
and skip setting properties that were initialized by the constructor.
The assumption is that if the constructor has a parameter with the same
name as the property, the property is set by the constructor. Otherwise
it needs to be set using a setter.

This matching was not implemented for the non-default constructor mechanism.
This commit fixes that.

Ladicek and others added 4 commits February 16, 2023 10:15
Bytecode recorders attempt to match constructor parameters to properties
and skip setting properties that were initialized by the constructor.
The assumption is that if the constructor has a parameter with the same
name as the property, the property is set by the constructor. Otherwise
it needs to be set using a setter.

This matching was not implemented for the non-default constructor mechanism.
This commit fixes that.
- Change creation of a large collection in `BytecodeRecorderTestCase.java` from using a loop to using `Collections.nCopies`
Bytecode recorders attempt to match properties to constructor parameters
by the property/parameter name. However, parameters do not always have
to have their names available. This commit fixes the bytecode recorder
matching mechanism to avoid parameters that do not have an available name.
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 16, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@gastaldi gastaldi changed the title [2.13] Fix the non-default constructor mechanism of bytecode recording Fix the non-default constructor mechanism of bytecode recording [2.13] Feb 16, 2023
@gsmet
Copy link
Member

gsmet commented Feb 16, 2023

@rsvoboda what's your take on this one for 2.13? It's a bug and could happen for people writing their own extensions.

@rsvoboda
Copy link
Member

Why was this dedicated PR created when #31183 has 2.13 backport label?

@gastaldi
Copy link
Contributor Author

@rsvoboda because it doesn't apply cleanly

@rsvoboda
Copy link
Member

So the backport label should be removed from #31183

@rsvoboda
Copy link
Member

I don't have comments regarding the code changes.

On the other hand it's kinda expected that people write extensions against the latest community.

jobrunr is using 2.15.3.Final - https://github.com/jobrunr/jobrunr/blob/master/gradle.properties#L7
And it was the one (and only one sofar?) complaining.

So I'm not super convinced that we need to merge this for 2.13.

@rsvoboda
Copy link
Member

(but still open to the discussion ;) )

@gastaldi
Copy link
Contributor Author

I'm okay in not backporting this

@gastaldi
Copy link
Contributor Author

I'll close it for now

@gastaldi gastaldi closed this Feb 16, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Feb 16, 2023
@gastaldi gastaldi deleted the bytecode_213 branch February 16, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants