Skip to content

fix: handle nested java types like Flow.Publisher in Util.shortName #826

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

Merged
merged 1 commit into from
Jun 8, 2025

Conversation

VincentPolfliet
Copy link
Contributor

This PR fixes a bug in Util.shortName(...) where nested types like java.util.concurrent.Flow.Publisher were resolved incorrectly as Publisher, which caused generated code to fail compilation when the short name was not imported correctly.

@SentryMan SentryMan enabled auto-merge June 8, 2025 15:39
Copy link
Collaborator

@SentryMan SentryMan left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

@SentryMan SentryMan merged commit 9c4302a into avaje:master Jun 8, 2025
5 checks passed
@VincentPolfliet VincentPolfliet deleted the fix/nested-type-shortname branch June 8, 2025 18:34
@SentryMan SentryMan added the bug Something isn't working label Jun 8, 2025
@SentryMan SentryMan added this to the 11.6 milestone Jun 8, 2025
@rbygrave
Copy link
Contributor

rbygrave commented Jun 8, 2025

Nice, but there is insufficient test coverage here and this PR should NOT have been merged without that coverage @SentryMan

@SentryMan
Copy link
Collaborator

SentryMan commented Jun 8, 2025

Nice, but there is insufficient test coverage here and this PR should NOT have been merged without that coverage @SentryMan

Is there something missing? Looks 100% to me with all code paths hit.
image

The problem described was not about wiring, but the generation of invalid code. We already have tests for wiring things with GenericType

rbygrave added a commit that referenced this pull request Jun 9, 2025
- Move the test objects GenericImpl, GenericInterfaceObject to blackbox-test-inject
- Long term it is not sufficient to compile but actually have a test that confirms it wires
- Also tidy UtilTest to merge the tests for shortName() + add some java types to that test
@rbygrave
Copy link
Contributor

rbygrave commented Jun 9, 2025

Is there something missing? Looks 100% to me with all code paths hit.

Yeah I was wrong. Test coverage there was ok. I was concerned about missing coverage on the java types but was wrong there.

SentryMan added a commit that referenced this pull request Jun 9, 2025
Test only followup for #826, add test to blackbox-test-inject
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants