Skip to content

Comments

Fix recursive polymorphism definition resolution#22

Open
tomjw64 wants to merge 1 commit intomasterfrom
twilliamson-1
Open

Fix recursive polymorphism definition resolution#22
tomjw64 wants to merge 1 commit intomasterfrom
twilliamson-1

Conversation

@tomjw64
Copy link

@tomjw64 tomjw64 commented Feb 21, 2025

This should fix two issues:

  1. Because the shortRef was originally generated via getDefinitionName but did not consider that classes sharing a simple name can have suffixes in their long and short ref names, the resolution could lead to issues where a definition for a class in one package was being overridden by a definition for a class sharing the same simple name in another package.
  2. Definitions were being set with definitionsNode.set(ref, w.nodeInProgress) instead of definitionsNode.set(shortRef, w.nodeInProgress) for in progress nodes, which would lead to duplicate definitions in the final schema (one with the long ref, one with the short ref). This call also appears to be redundant, since if a definition is in progress, it should always be added to the definitions set, even across runs, but it was left in place, but with a check to alwaysReturnDefinitions, in case I missed something.

Context:
772dc8b
https://hubspot.slack.com/archives/C08ADS1C887/p1740094210967859
https://hubspot.slack.com/archives/C5QNWTWFJ/p1740139578561809

@tomjw64 tomjw64 force-pushed the twilliamson-1 branch 2 times, most recently from b782eb9 to 28f9f34 Compare February 21, 2025 20:57
@tomjw64 tomjw64 changed the title Only add recursive polymorphism definitions when alwaysReturnDefinitions Fix recursive polymorphism definition resolution Feb 21, 2025
@tomjw64 tomjw64 marked this pull request as ready for review February 21, 2025 21:13
Copy link
Collaborator

@zrrobbins zrrobbins left a comment

Choose a reason for hiding this comment

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

Your changes make sense to me, although have you confirmed this works as expected for your use case. I.e. locally installing the updated library and running whatever project is dependent on it?

On that note as well, I'll tag the API Tooling Team here (@elangan @CyanCode) as that was what I originally imported/tweaked this library's use cases for. They may want to test the updated library themselves to ensure Swagger generation still performs as expected.

@tomjw64
Copy link
Author

tomjw64 commented Feb 24, 2025

Your changes make sense to me, although have you confirmed this works as expected for your use case. I.e. locally installing the updated library and running whatever project is dependent on it?

I have! I've been using the resulting branch build 1.0-mbknor-jackson-jsonschema_2.12-1.8-twilliamson-1-SNAPSHOT for my own usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants