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.

Sign up for GitHub

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

Jump to bottom

Allow use of abstract classes in Quarkus REST in the same way as interfaces #41606

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Allow use of abstract classes in Quarkus REST in the same way as interfaces #41606

merged 1 commit into from
Jul 3, 2024

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jul 2, 2024

@geoand
Copy link
Contributor Author

geoand commented Jul 2, 2024

@michalvavrik would you like to test this and see if it fixes the various use cases you had in mind?

It does fix the one you linked you, but IIUC you had more.

@michalvavrik
Copy link
Member

@michalvavrik would you like to test this and see if it fixes the various use cases you had in mind?

It does fix the one you linked you, but IIUC you had more.

I would. I'll be back in 2 hours and test it. Thanks for the fix.

@geoand
Copy link
Contributor Author

geoand commented Jul 2, 2024

Thanks! No rush whatsoever

michalvavrik
michalvavrik approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

Fixed all my issues, thanks!

BTW. tests that are fixed by this will be part of my next PR for endpoint. impl. security checks in Quarkus REST

@geoand
Copy link
Contributor Author

geoand commented Jul 2, 2024

Excellent, thanks!

@geoand geoand marked this pull request as ready for review July 2, 2024 17:52
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@geoand geoand requested a review from gastaldi July 3, 2024 05:43
@gastaldi
Copy link
Contributor

gastaldi commented Jul 3, 2024

Shall we backport this fix too?

gastaldi
gastaldi reviewed Jul 3, 2024
View reviewed changes
...main/java/org/jboss/resteasy/reactive/common/processor/scanning/ResteasyReactiveScanner.java Outdated
// handle abstract classes
Set<ClassInfo> abstractScannedResources = scannedResources.values().stream().filter(ClassInfo::isAbstract)
.collect(Collectors.toSet());
for (ClassInfo abstractScannedResource : abstractScannedResources) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe avoid the extra iteration above and call isAbstract here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you are referring to here

Copy link
Contributor

Choose a reason for hiding this comment

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

You are creating a Set above just to iterate again in this loop, I am suggesting merging both loops to a single one

@geoand
Copy link
Contributor Author

geoand commented Jul 3, 2024

Shall we backport this fix too?

sure yeah

@geoand geoand merged commit 229b1c0 into quarkusio:main Jul 3, 2024
46 checks passed
@geoand geoand deleted the #41567 branch July 3, 2024 14:13
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 3, 2024
@michalvavrik michalvavrik mentioned this pull request Jul 6, 2024
Merged
@gsmet gsmet modified the milestones: 3.13 - main, 3.12.2 Jul 9, 2024
@gsmet gsmet modified the milestones: 3.12.2, 3.8.6 Aug 14, 2024
rsvoboda added a commit to rsvoboda/quarkus-test-suite that referenced this pull request Sep 2, 2024
@rsvoboda
Test Quarkus REST abstract resources with @path
81f25b7 
Triggered by quarkusio/quarkus#41606
This was referenced Sep 2, 2024
Merged
Open
rsvoboda added a commit to rsvoboda/quarkus-test-suite that referenced this pull request Sep 3, 2024
@rsvoboda
Test Quarkus REST abstract resources with @path
47de22c 
Triggered by quarkusio/quarkus#41606
rsvoboda added a commit to rsvoboda/quarkus-test-suite that referenced this pull request Sep 3, 2024
@rsvoboda
Test Quarkus REST abstract resources with @path
214fdfe 
Triggered by quarkusio/quarkus#41606
michalvavrik pushed a commit to michalvavrik/quarkus-test-suite that referenced this pull request Sep 7, 2024
@rsvoboda @michalvavrik
Test Quarkus REST abstract resources with @path
e63a308 
Triggered by quarkusio/quarkus#41606

(cherry picked from commit 214fdfe)
michalvavrik pushed a commit to michalvavrik/quarkus-test-suite that referenced this pull request Sep 7, 2024
@rsvoboda @michalvavrik
Test Quarkus REST abstract resources with @path
3edf058 
Triggered by quarkusio/quarkus#41606

(cherry picked from commit 214fdfe)
@rsvoboda rsvoboda mentioned this pull request Sep 10, 2024
Merged
michalvavrik pushed a commit to michalvavrik/quarkus-test-suite that referenced this pull request Sep 13, 2024
@rsvoboda @michalvavrik
Test Quarkus REST abstract resources with @path
c07d1bb 
Triggered by quarkusio/quarkus#41606

(cherry picked from commit 214fdfe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@michalvavrik michalvavrik michalvavrik approved these changes

Assignees
No one assigned
Labels
kind/bugfix
Projects
None yet
Milestone
3.8.6
Development

Successfully merging this pull request may close these issues.

Quarkus REST abstract resources with @Path requires impl. to be CDI beans while RESTEasy does not
4 participants
@geoand @michalvavrik @gastaldi @gsmet