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

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

baywet
Copy link
Member

@baywet baywet commented Dec 31, 2024

reviewers, please review #2032 first.

@baywet baywet self-assigned this Dec 31, 2024
@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.

@baywet baywet added this to the v2 - preview5 milestone Jan 8, 2025
Base automatically changed from fix/tag-references to dev January 15, 2025 21:46
darrelmiller
darrelmiller previously approved these changes Jan 15, 2025
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

@baywet baywet merged commit d7c4621 into dev Jan 16, 2025
11 of 12 checks passed
@baywet baywet deleted the fix/missing-document-references branch January 16, 2025 13:09
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.

3 participants