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 union sub-types are not added to the union #28

Merged
merged 1 commit into from
Jul 4, 2022

Conversation

sGy1980de
Copy link
Contributor

This PR is about to fix an issue with union types, I stumbled over. Since v0.0.17 this was broken for me, because the union type appeared to be empty, at least for the introspection.

I boiled down the issue to be introduced in PR #19. I guess it was a simple typo back then, since the if clause is currently decoupled from the loop.

I modified the test as well, to ensure the union.Types slice is actually populated

@JohnStarich
Copy link
Member

Thanks for the PR @sGy1980de! Really appreciate the test case too.

Can you provide an example with an expected result? I think I'm understanding your change, but I don't quite understand the scenario yet.

Is the issue occurring when introspecting a schema with something like this?

union Animal = Animal | Human

Looking at the spec on Union Type validation, a self-referencing Union would fail validation since it only permits Objects (type) as members of a Union. Looks like this change may then ignore the malformed Union?
I could be misunderstanding the problem you hit too, let me know!

@sGy1980de
Copy link
Contributor Author

Hey @JohnStarich, thank you for the fast response. I really appreciate your engagement in this project. It got a lot of new momentum.

BTT:
Perhaps you got my PR wrong. In our projects, I pinned the version of this lib to v0.0.16, since this check for the types name was introduced in #19 and went live with v0.0.17. This one breaks the union types for us. Why @danielvladco introduced this check back then, I have no idea. I just assume, he used the wrong variable to check against, remoteType vs. possibleType, and therefore basically drove union.Types useless, because it never got populated. The original tests pass, regardless which variable is used for the check. This is because the contents of union.Types are not asserted at all.

If you ask me, the check is useless, as you said, the spec forbids such a scenario. Perhaps we should simply remove the if-clause then?

Copy link
Member

@JohnStarich JohnStarich left a comment

Choose a reason for hiding this comment

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

Thanks @sGy1980de, I'm understanding now. Looks like that remoteType vs possibleType piece is the real kicker for the wrong Types slice.

After digging in a bit further, it appears the if-clause is needed since "possible types" can include itself. It appears to be a way to see which types "match" each other. As I understand it, types are equal to themselves, so that's why they appear in their own possible types.

Thanks again!

@JohnStarich JohnStarich merged commit ba9fbc9 into nautilus:master Jul 4, 2022
JohnStarich added a commit that referenced this pull request Jul 4, 2022
JohnStarich added a commit that referenced this pull request Jul 4, 2022
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.

None yet

2 participants