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

openapi3: improve internalization ref naming to avoid collisions #955

Merged
merged 16 commits into from
Jul 3, 2024

Conversation

percivalalb
Copy link
Contributor

@percivalalb percivalalb commented Jun 2, 2024

#952

Internalizating references is the way in which a spec which references external files can be shipped as one combined document. The current implementation in DefaultRefNameResolver uses just the final part of the $ref. That either being the name of the fixed field component or file name of a whole document.

This works in most specs if they maintain documents in the same directory but as soon as you bring child directories the chance of a comflicting file name increasing meaning that two distinct schemas might resolve to the same internalized name. This then goes unnoticed and you unwittingly ship an incorrect spec. Also due to some indeterminisitc logic internalisting the references either of the distinct schemas can be internalized first which leads to an inconistent incorrect spec. (I've refered to schema's here but this appies to all the component fixed field types).

This MR:

  • Keeps track of the document paths relative to the root document.
  • These paths are then used to generate the internalized name using the entire path to avoid collisions.
  • Makes the parsing and interlization deterministic by iterating over maps (which have no defined order) in sorted string key order.
  • The DefaultRefNameResolver has been switched to the new logic, which therefore includes an API breaking change & internalised spec changes.

Like before the interlization code doesn't warn if you provide a "name internalizer" that produces colision - this could be added in a follow-up PR. Work would need think out about that could be done.

@percivalalb percivalalb changed the title Draft: Improve internalisation Draft: Improve internalisation ref naming to avoid collisions Jun 2, 2024
@ccoVeille
Copy link

Great changes!

One small remark: unless if the project is totally non-US based (so using colour, behaviour, and company), and that's commonly and widely accepted, I would have expected the whole code, issues and PR to mention : internalize (and all variations) instead of internalise

Please note, I'm French. It's the kind of error I used to do, so I spotted it

// the existing component.
if nameInRoot, found := MatchesComponentInRootDocument(doc, ref); found {
c := *ref.RefPath()
c.Path = nameInRoot

Check warning

Code scanning / CodeQL

Useless assignment to field

This assignment to Path is useless since its value is never read.
@percivalalb
Copy link
Contributor Author

One small remark: unless if the project is totally non-US based (so using colour, behaviour, and company), and that's commonly and widely accepted, I would have expected the whole code, issues and PR to mention : internalize (and all variations) instead of internalise

Thanks for pointing out, I had it in the back of my mind but wasn't actively making sure I used the American spelling. I'm English so do this automatically too :D

@percivalalb percivalalb changed the title Draft: Improve internalisation ref naming to avoid collisions Draft: Improve internalization ref naming to avoid collisions Jun 23, 2024
@percivalalb percivalalb force-pushed the alb/ref-issues branch 6 times, most recently from 8091a0a to 8e50176 Compare June 28, 2024 05:57
openapi3/refs.tmpl Outdated Show resolved Hide resolved
@percivalalb
Copy link
Contributor Author

I think this is very close now, if you'd like to do a full review @fenollp then please go ahead (you may have already as I've been trundling along). I'll do another pass through later this evening too.

Copy link
Collaborator

@fenollp fenollp left a comment

Choose a reason for hiding this comment

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

LGTM! Please rebase and use v0.126.0 as the release version in README and we'll be good to go :)

Still got some work to do, the recursive test still fails
This makes resolving references & internalising references determinstic
by sorting map for loops by key.

Ensures refs are resolved in the same order, depending on the spec this
can result in a different (but equal value) internalised spec.
The unmarshal function was removing the .url value
This will be the path at the closest point to the actual definition
in the reference chain.

Also trim . from the start of paths
Excuse my British English
@percivalalb percivalalb changed the title Draft: Improve internalization ref naming to avoid collisions Improve internalization ref naming to avoid collisions Jul 3, 2024
@percivalalb percivalalb requested a review from fenollp July 3, 2024 06:48
@fenollp fenollp changed the title Improve internalization ref naming to avoid collisions openapi3: improve internalization ref naming to avoid collisions Jul 3, 2024
@fenollp fenollp merged commit 0ed9f5d into getkin:master Jul 3, 2024
5 checks passed
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