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

Warn about interceptors on private methods #21260

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 8, 2021

Resolves: #21046

P.S. Another solution would be to remove the private modifier from the method same as we do for final, but I am not sure if we want that.

@geoand geoand requested a review from mkouba November 8, 2021 08:21
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Nov 8, 2021
@mkouba
Copy link
Contributor

mkouba commented Nov 8, 2021

P.S. Another solution would be to remove the private modifier from the method same as we do for final, but I am not sure if we want that.

-1

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 8, 2021
@famod
Copy link
Member

famod commented Nov 8, 2021

Thanks a lot for this! It's too easy to run into this problem, e.g. when refactoring existing code.

@geoand
Copy link
Contributor Author

geoand commented Nov 8, 2021

Indeed it is :)

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 8, 2021

Failing Jobs - Building d8d8305

Status Name Step Failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build Failures Logs Raw logs

Failures

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

@geoand geoand merged commit 21e76ea into quarkusio:main Nov 8, 2021
@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Nov 8, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 8, 2021
@geoand geoand deleted the #21046 branch November 8, 2021 13:28
@famod
Copy link
Member

famod commented Dec 14, 2021

@geoand I colleague just lost quite a bit of time because he had @Tranasctional(REQUIRES_NEW) on a private method and he didn't see the warning (I did spot it quickly when he asked for help but I guess not everyone is so "sensitive" to warnings).

So I'm wondering if we should provide a config property to control whether to just warn or throw an exception?

@geoand
Copy link
Contributor Author

geoand commented Dec 14, 2021

Sure, that sounds like a good idea

@geoand
Copy link
Contributor Author

geoand commented Dec 14, 2021

Do you or your colleague want to open a PR for that? I can certainly do it.

@famod
Copy link
Member

famod commented Dec 14, 2021

You are very welcome to do that! We are a bit in a rush for a release next week.

@geoand
Copy link
Contributor Author

geoand commented Dec 14, 2021

Hm... Actually the idea of this sort of conflicts with the quarkus.arc.transform-unproxyable-classes.
We don't try to transform the private method case (and we do mention this in the docs of the property), but I am not sure it will be intuitive to users if we have something like quarkus.arc.transform-unproxyable-classes=true and quarkus.arc.fail-on-private-interceptors=true...

@mkouba WDYT?

@mkouba
Copy link
Contributor

mkouba commented Dec 14, 2021

Well, private intercepted observer/producer methods are legal and should work just fine so we would have to ignore these.

but I am not sure it will be intuitive to users if we have something like quarkus.arc.transform-unproxyable-classes=true and quarkus.arc.fail-on-private-interceptors=true...

Private methods don't turn a class into an unproxyable type so it should be ok I think...

@geoand
Copy link
Contributor Author

geoand commented Dec 14, 2021

Understood, thanks

@geoand
Copy link
Contributor Author

geoand commented Dec 14, 2021

#22185 takes care of it

geoand added a commit to geoand/quarkus that referenced this pull request Dec 14, 2021
geoand added a commit to geoand/quarkus that referenced this pull request Dec 15, 2021
geoand added a commit to geoand/quarkus that referenced this pull request Dec 15, 2021
geoand added a commit that referenced this pull request Dec 15, 2021
Provide Arc configuration allowing to fail when interceptors used on private methods
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quarkus instrumentation warns about use of @Transactional on private methods
3 participants