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

fix: passes missing host document references to all layers #2033

Open
wants to merge 1 commit into
base: fix/tag-references
Choose a base branch
from

Conversation

baywet
Copy link
Member

@baywet baywet commented Dec 31, 2024

reviewers, please review #2032 first.

@baywet baywet self-assigned this Dec 31, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
62.7% Coverage on New Code (required ≥ 80%)
44.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@darrelmiller
Copy link
Member

I'm not sure we have to pass the host document through every layer as I think we can store it in the ParsingContext object which is accessible via the node object. I also think we may need to store the "Entry Document" also, when dealing with multi-document descriptions.

Security Schemes and Tags are only supposed to be resolved from the "Entry Document" and not the Host document.

@baywet
Copy link
Member Author

baywet commented Jan 6, 2025

@darrelmiller if you look at a high level at this PR changes you'll noticed that all of them are for deserialization scenarios. Before this PR: deserialize a document, try to access the object model, references are not getting resolved. This is a really broken experience.

I think part of the reason why references were not being set properly is because of the optional parameters: the compiler was not yelling at the developer something is missing. I expect the vast majority of package consumers will never use those methods. But for those who do, I think it's a better experience to tell them "hey, make sure you provide what we need to give you a valid document later" rather than telling them "do whatever you want" and blow up in their faces later on. Another aspect that comforts me in this position is that references in their current form are extremely hard to debug by a consumer unless you link the sources. Which we can't expect anymore but us and a few advanced developers to do.

If you think those changes break things for external documents (or other scenarios), please provide test cases/examples I can incorporate with this pull request.

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