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: InputDict dynamicMember operator conflict with hash function #2607

Merged
merged 11 commits into from
Oct 26, 2022

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Oct 25, 2022

Fixes #2603

If you have an InputObject with a parameter name of hash it will conflict with the Hashable protocol function named hash. We can fix this by using the fully qualified operator name for the dynamicMember subscript removing dynamic member lookup and using a custom subscript.

@netlify
Copy link

netlify bot commented Oct 25, 2022

Deploy Preview for apollo-ios-docs canceled.

Name Link
🔨 Latest commit ed7be04
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docs/deploys/63597e93289e670008b5888a

@AnthonyMDev
Copy link
Contributor

I assume this will be the same issue for SelectionSet if you have a field named hash then? I don't love the verbosity of adding this qualified member name to every field accessor though.

I'd rather special case the keyword hash and just use data[dynamicMember: "hash"] in the specific case of this. It may be that there are other keywords which will cause this problem also that we have not encountered yet (or that will become issues in the future if we add to the SelectionSet/InputObject APIs). And we should have a general solution for this built into SwiftKeywords for this.

@calvincestari calvincestari force-pushed the fix/inputdict-dynamicmember branch 2 times, most recently from fd9eff7 to 6f5fcea Compare October 26, 2022 04:58
@calvincestari calvincestari force-pushed the fix/inputdict-dynamicmember branch from 6f5fcea to 1362c67 Compare October 26, 2022 06:14
@calvincestari calvincestari force-pushed the fix/inputdict-dynamicmember branch from 1362c67 to b6bb519 Compare October 26, 2022 07:20
@calvincestari
Copy link
Member Author

I assume this will be the same issue for SelectionSet if you have a field named hash then? I don't love the verbosity of adding this qualified member name to every field accessor though.

I checked and no this issue doesn't exist for SelectionSet because DataDict does not offer @dynamicMemberLookup. It instead implements a custom subscript and avoids this issue.

That gave me the idea of simply making InputDict more like DataDict and to remove @dynamicMemberLookup, and from what I can tell we don't lose anything and manage to avoid needing to use dynamicMember:.

I've added a field named hash to AnimalKingdomAPI in both an input and selection set and with all generated code updated it all builds and functions as expected. Can you think of any reason we'd need to keep @dynamicMemberLookup?

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

This is definitely the right way to handle this. Nice!

Nit: I'm not a fan of adding properties to test edge cases to our example generated models. I add things to them while I'm testing things out, but then I write a unit test and remove the test properties from the main generated models. This really isn't a big deal, and if you want to keep it, I'm not going to argue it. But I would hesitate to make this a common practice. These are meant to be an example of the basic structure, so that our users can see an example of what good generated models look like. But if we start using them to maintain regression testing of edge cases, they will become ugly and not a good quality example for people. And they also conflict with the point of unit tests. I don't want us getting into a habit of relying on "if the example models still build, then it's okay." because that logic leads to lazier unit testing habits.

@calvincestari
Copy link
Member Author

I had this test which I removed - d766a29.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants