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

ArC: prevent removing the final flag from records #33882

Merged
merged 2 commits into from
Jun 9, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jun 7, 2023

Record classes must always be final, even the Class.isRecord() method checks that. Therefore, the bytecode transformation that removes the final flag from a class is invalid for records.

With this commit, ArC will no longer blindly remove the final flag from records; a deployment problem occurs instead. This means that records cannot be used to define normal scoped beans and cannot be intercepted or decorated.

Fixes #33810

@Ladicek Ladicek requested review from mkouba and manovotn June 7, 2023 15:40
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jun 7, 2023
@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 7, 2023

I would have added a test, but I don't think we have a good place for that.

We could add independent-projects/arc/tests-java17 maybe. Or perhaps even independent-projects/arc/tests/src/test/java17?

@quarkus-bot

This comment has been minimized.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 8, 2023

I'll try adding the java17 source directory then. We already have src/test/java and src/test/kotlin, so src/test/java17 seems most natural.

Ladicek added 2 commits June 8, 2023 12:10
Record classes must always be `final`, even the `Class.isRecord()`
method checks that. Therefore, the bytecode transformation that
removes the `final` flag from a class is invalid for records.

With this commit, ArC will no longer blindly remove the `final`
flag from records; a deployment problem occurs instead. This means
that records cannot be used to define normal scoped beans and
cannot be intercepted or decorated.
@Ladicek Ladicek force-pushed the prevent-removing-final-from-records branch from d3179dc to 540b809 Compare June 8, 2023 10:10
@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 8, 2023

Done, tests are in the 2nd commit.

@manovotn manovotn added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 8, 2023
@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 8, 2023

Failing Jobs - Building 540b809

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 19 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 19 #

- Failing: extensions/amazon-lambda/deployment 
! Skipped: extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment integration-tests/amazon-lambda and 6 more

📦 extensions/amazon-lambda/deployment

io.quarkus.amazon.lambda.deployment.testing.LambdaDevServicesContinuousTestingTestCase.testLambda line 48 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <1> but was: <0>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 9, 2023

LambdaDevServicesContinuousTestingTestCase.testLambda is known flaky, gonna merge this.

@Ladicek Ladicek merged commit 76f4d9b into quarkusio:main Jun 9, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 9, 2023
@Ladicek Ladicek deleted the prevent-removing-final-from-records branch June 9, 2023 06:48
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) kind/bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A normal scoped bean implemented as Java record cannot be serialised to json
3 participants