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.

Sign up for GitHub

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

Jump to bottom

Add a flag to disable fieldsCanMerge validation on disjoint types #4342

Merged
merged 5 commits into from
Aug 27, 2022
Merged

Add a flag to disable fieldsCanMerge validation on disjoint types #4342

merged 5 commits into from
Aug 27, 2022

Conversation

pt2121
Copy link
Contributor

@pt2121 pt2121 commented Aug 14, 2022 •
edited
Loading

re #4320

Basically, I skip fieldsInSetCanMerge if the fieldOnDisjointTypesMustMerge flag is true.
Please let me know if that's the expectation.

@apollo-cla
Copy link

@pt2121: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@netlify
Copy link

netlify bot commented Aug 14, 2022 •
edited
Loading

👷 Deploy request for apollo-android-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9858039

@pt2121 pt2121 force-pushed the pt/fieldOnDisjointTypesMustMerge-4320 branch 3 times, most recently from 26276ff to 6c7cf00 Compare August 14, 2022 15:41
@pt2121 pt2121 marked this pull request as ready for review August 14, 2022 15:56
BoD
BoD reviewed Aug 16, 2022
View reviewed changes
Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

Thank you for this!

A few comments, but looks good overall.

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/Options.kt Outdated Show resolved Hide resolved
apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/gqloperationdefinition.kt Outdated Show resolved Hide resolved
apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/api.kt Outdated Show resolved Hide resolved
apollo-compiler/src/test/kotlin/com/apollographql/apollo3/compiler/ValidationTest.kt Outdated Show resolved Hide resolved
apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/internal/ExecutableValidationScope.kt Outdated Show resolved Hide resolved
@pt2121
Copy link
Contributor Author

pt2121 commented Aug 16, 2022

Thanks for the feedback! I will look into them tonight.

@pt2121 pt2121 marked this pull request as draft August 17, 2022 03:56
@pt2121 pt2121 force-pushed the pt/fieldOnDisjointTypesMustMerge-4320 branch from 66eaeca to e8313e9 Compare August 17, 2022 04:53
@pt2121 pt2121 force-pushed the pt/fieldOnDisjointTypesMustMerge-4320 branch from e8313e9 to 4d04041 Compare August 18, 2022 04:32
@pt2121 pt2121 force-pushed the pt/fieldOnDisjointTypesMustMerge-4320 branch from 4d04041 to 641f5f9 Compare August 18, 2022 04:37
@pt2121 pt2121 marked this pull request as ready for review August 18, 2022 04:37
@pt2121 pt2121 requested review from BoD and removed request for sav007 and martinbonnin August 18, 2022 04:38
BoD
BoD approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This looks good 👍

martinbonnin
martinbonnin reviewed Aug 24, 2022
View reviewed changes
Copy link
Contributor

@martinbonnin martinbonnin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few comments but looks good to me overall. Let me know what you think.

On a separate note, reviewing this, I realized we have a bug in the validation logic and something like below doesn't throw a validation error:

type Query {
  value: Int
  value2: Int
}
{
  value
  value: value2
}

A server should (rightfully) forbid this. Since this is permissive at the moment, I don't want to break other people workflows by making this more strict but this is certainly something to revisit for v4

@martinbonnin martinbonnin mentioned this pull request Aug 24, 2022
Closed
33 tasks
@pt2121
Copy link
Contributor Author

pt2121 commented Aug 26, 2022

Martin, thanks for the review. I'll look into them this weekend.

@martinbonnin
Copy link
Contributor

Thanks for the contribution! I just launched the CI tests. I'll merge once everything is ✅

@pt2121
Copy link
Contributor Author

pt2121 commented Aug 27, 2022 •
edited
Loading

Sounds good, thanks for a small ticket for a new contributor 🙂

@pt2121
Copy link
Contributor Author

pt2121 commented Aug 27, 2022 •
edited
Loading

Ah, test failed somehow. 😅
It looks Ok when I ran on my local. let me recheck

@martinbonnin
Copy link
Contributor

That's a flake, I've seen it before (see also #3920). I just relaunched the CI, it should pass this time

@martinbonnin martinbonnin merged commit 9ee4ae5 into apollographql:main Aug 27, 2022
@martinbonnin
Copy link
Contributor

It's all merged. Thanks again for the contribution!

@pt2121 pt2121 deleted the pt/fieldOnDisjointTypesMustMerge-4320 branch August 27, 2022 18:43
@bignimbus bignimbus mentioned this pull request Jul 25, 2024
Open
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinbonnin martinbonnin martinbonnin left review comments

@BoD BoD BoD approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pt2121 @apollo-cla @martinbonnin @BoD