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

Conflicting field names on input object #3257

Closed
alexifrim opened this issue Oct 18, 2023 · 7 comments · Fixed by apollographql/apollo-ios-dev#137
Closed

Conflicting field names on input object #3257

alexifrim opened this issue Oct 18, 2023 · 7 comments · Fixed by apollographql/apollo-ios-dev#137
Assignees
Labels
codegen Issues related to or arising from code generation feature New addition or enhancement to existing solutions low-priority planned-next Slated to be included in the next release

Comments

@alexifrim
Copy link

alexifrim commented Oct 18, 2023

Use case

At the moment, if for some reason there's a field named book and a relationship named Book (I know, bad, but we can't always control the schema), we end up with a conflict (redeclaration).

I think there should be an option (e.g. none) such that the field names are kept as they are (except for special swift keywords). Changing the case can easily lead to conflicts in plenty of situations.

Describe the solution you'd like

A new enum case (none) should be added to FieldAccessors, while this line would go under case .idiomatic
https://github.com/apollographql/apollo-ios/pull/3171/files#diff-43df1d4b245bbf664b141527621ce12de522a1fa92789bd87d4f39d8f5ef6865R52

@alexifrim alexifrim added the feature New addition or enhancement to existing solutions label Oct 18, 2023
@calvincestari
Copy link
Member

Hi @alexifrim - you could use a GraphQL field alias to rename the field and the generated model would take on the alias name instead.

@AnthonyMDev
Copy link
Contributor

In additional to field aliases, we are also planning a future feature that will allow you to alter the generated names of schema types. So that might help solve this issue from the other end.

Not that we are necessarily against adding this .none option, but I'd like to get more feedback here first. Knowing that there are field aliases as a current workaround, and we are planning a feature to allow changing names of generated schema types in the future, do you still feel like this is a valuable addition @alexifrim?

I feel like it's a very broad approach, as with none, you're turning off idiomatic naming for every field in your entire generated model set to fix a redeclaration error with one field. Wouldn't it be better to just configure the one field?

@alexifrim
Copy link
Author

alexifrim commented Oct 19, 2023

A field alias won't work, as the issue is when generating the schema types - more specifically, this ends up as an issue inside the [type]_bool_exp (not part of a query fetch result - but when having a generic where clause on a table that also has relationships to various other tables)
e.g.

struct Something_bool_exp: InputObject {
...
  init() {
    book: GraphQLNullable<Books_bool_exp> = nil,
    ...
    book: GraphQLNullable<String_comparison_exp> = nil,
  }
}

But yes, an option to alter the generated name of schema type (as long as it's also possible to distinguish between relationships and fields), should also work as long as it's isolated cases that cause the conflicts.

I think that it's great to have a layer that can change the generated field names, but in my opinion, at least for the casing, there should be an option to allow generating names as they are.

@calvincestari calvincestari added the codegen Issues related to or arising from code generation label Oct 19, 2023
@AnthonyMDev
Copy link
Contributor

So, it looks to me like this is about the names of the fields on your input object, rather than the names of the actual types. Please clarify if I'm wrong on that.

It looks to me like the issue here is that your schema has an input object that looks something like this:

input Something_bool_exp {
  Book: Books_bool_exp
  book: String_comparison_exp
}

This isn't about the names of the types (Books_bool_exp & String_comparison_exp), but about the names of the fields on the input object. Book and book. Is that correct?

@alexifrim
Copy link
Author

Yes - that is correct, sorry for the misunderstanding, I thought they're related

@AnthonyMDev
Copy link
Contributor

Thanks for the confirmation. We'll have to consider how best to address this issue. This is an edge case we hadn't yet had reported, so thanks for bringing it to our attention. I don't think there is really a workaround for you in the mean time. So this is something we need to address.

@AnthonyMDev AnthonyMDev changed the title fieldAccessors option that doesn't apply any changes to the field names Conflicting field names on input object Oct 23, 2023
@AnthonyMDev AnthonyMDev added this to the Patch Releases (1.x.x) milestone Oct 23, 2023
@AnthonyMDev AnthonyMDev added the planned-next Slated to be included in the next release label Oct 23, 2023
@AnthonyMDev
Copy link
Contributor

We're going to add a conversion strategy that mimics the enum case conversion strategy to account for this edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Issues related to or arising from code generation feature New addition or enhancement to existing solutions low-priority planned-next Slated to be included in the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants