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 type reflection for repeated custom types #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jpalawaga
Copy link

@jpalawaga jpalawaga commented Mar 6, 2024

See discussion in #194

@jpalawaga
Copy link
Author

@vearutop ping! if you're busy, np, I can just work off of the fork.

@vearutop
Copy link
Member

TL;DR this does not seem like a fix to anything.

I think you've highlighted a bug in the current test.

For JSON schema up to draft-07 (OpenAPI 3 is based on draft-04 behavior to my knowledge) any keywords adjacent to $ref should be ignored. See this issue for relevant discussion.

OpenAPI 3.1 is based on a recent JSON schema, so probably adjacent keywords should not be a problem.

This library is draft-07 compliant, so I think we should try to avoid $ref adjacent keywords or workaround them with allOf (at least by default).

One way to "solve" the original issue is to get rid of the $ref altogether by inlining the definition with SchemaInliner.

@jpalawaga
Copy link
Author

Ah, yeah, I see that this causes the rest of the machinery to pull the full schema instead of just the reference.

From the link you provide, it does seem like the library is currently doing the right thing by not placing adjacent properties on a ref value.

Though my follow-on thought is not to inline the schema definition. Recursive document exploration (with no short-circuiting) can get expensive depending on the size of the schema.

There is a keepType parameter on reflect, though I'm not sure it's intended purpose. I tried to move the typeKeep-ing into a defer statement that could decorate from the cached schema if it needed the type, though this seems to still break tests.

The root issue seems to be that walkProperty expects to have a full schema type returned by reflect but sometimes it just returns a ref.

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