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

Generates conflicting types for fields of singular and plural names #2850

Closed
sgade opened this issue Feb 24, 2023 · 7 comments · Fixed by #3009
Closed

Generates conflicting types for fields of singular and plural names #2850

sgade opened this issue Feb 24, 2023 · 7 comments · Fixed by #3009
Assignees
Labels
bug Generally incorrect behavior codegen Issues related to or arising from code generation planned-next Slated to be included in the next release
Milestone

Comments

@sgade
Copy link

sgade commented Feb 24, 2023

Summary

When querying different properties of similar name — one being singular, one being plural — with a different subset of properties , Apollo generates conflicting type names for the query response.

We have a query with two properties on a parent type. One is a single instance, the other is an array. Both are of the same type in the schema. When we query different subsets of properties on each, Apollo generated separate types (as expected) but names them the same. This is a conflict because they are defined on the same level.

Version

1.0.7

Steps to reproduce the behavior

Query example:

value {
  propertyA
  propertyB
  propertyC
  propertyD
}

values {
  propertyA
  propertyC
}

Both resulting types are called Value in the Swift project. This makes any reference ambiguous and fails the build.

Logs

No response

Anything else?

Apollo on Android seems to notice the conflicting type names, as it generates one Value and one Value1 type.

@sgade sgade added bug Generally incorrect behavior needs investigation labels Feb 24, 2023
@calvincestari
Copy link
Member

Is this a schema issue or rather a generation issue on both platforms?

If the schema + operation passes GraphQL validation then we need to support it and generate correct Swift code, unless in the rare case that the validation is actually wrong. That simple query above should yield types named SingleValue and MultipleValues respectively, with propertyA, etc. getting defined with the eventual scalar type. GraphQL doesn't allow two fields to have the same name so I don't follow where the same type is being generated.

It would be better if you can you share a standalone sample app demonstrating the issue; schema + operation + generated Swift code that doesn't compile.

@sgade
Copy link
Author

sgade commented Feb 24, 2023

Thanks for the reply. I will try to get you an example next week.

To clarify the example above: The types for singleValue is Value and for multipleValue it is [Value]. For both, Apollo generates a separate Value type in Swift but because they are on the same level and named identically, that is the issue.

@sgade
Copy link
Author

sgade commented Feb 27, 2023

I attached an example of the structure that causes the issue described. However, I does generate the types named the way you said SingleValue and MultipleValue in this isolated project. Does that help you in any way? Could it be related to anything else in the chain?

ConflictingTypesExample.zip

@sgade
Copy link
Author

sgade commented Feb 28, 2023

I found the root issue. My previous example was not close enough to our internal schema. I updated the original issue post accordingly.

The problem is that we have two properties, called value and values. They are both of type Value. Apollo then generates two conflicting types of Value for these. On Android, we found Apollo will generate Value and Value1, so there might already be some kind of conflict checking in place?

From the example project:

type Container {
  value: Value
  values: [Value]
}

type Value {
  propertyA: String!
  propertyB: String!
  propertyC: String!
  propertyD: String!
}
containers {
    value {
        propertyA
        propertyB
        propertyC
        propertyD
    }

    values { # <- this will generate another "Value" type, not a "Values" type.
        propertyA
        propertyC
    }
}

ConflictingTypesExample.zip


The workaround is to assign local aliases. Is this intended behaviour?

@sgade sgade changed the title Generates conflicting types for queries of the same type Generates conflicting types for fields of singular and plural names Feb 28, 2023
@calvincestari
Copy link
Member

The problem is that we have two properties, called value and values. They are both of type Value. Apollo then generates two conflicting types of Value for these.

The workaround is to assign local aliases. Is this intended behaviour?

The type being named Value in the schema is a distraction; the selection set types are named according to the property name, which is why using an alias works. Using thing and things as the field names would result in the same error. The root problem will be in our use of the pluralizer where types names are always singularized field names.

One of the guiding principles we have for codegen is what-you-see-is-what-you-get where the user is able to influence the shape of the generated code directly through the operation with as little 'magic' as possible. The ability to use an alias for resolving issues like this is intended but not very desirable, and IMO this issue is stretching that principle because you need to be aware of the implementation detail (singularized field names as type names) in order to understand how to fix it.

Apollo Kotlin is likely doing some conflict checking and working around it although I don't think Value1 is a very intuitive replacement name. I'll add this to the discussion queue and we'll see what we can do to resolve it.

@calvincestari calvincestari added codegen Issues related to or arising from code generation and removed needs investigation labels Mar 3, 2023
@sgade
Copy link
Author

sgade commented Mar 3, 2023

One of the guiding principles we have for codegen is what-you-see-is-what-you-get where the user is able to influence the shape of the generated code directly through the operation with as little 'magic' as possible.

I agree with that position and until now, I didn't have any problems with the type names generated by Apollo. Using the singular name makes sense in all other cases, so it should be kept.

However, this case is especially frustrating because it leaves the developer with a "ambiguous type reference" compiler error and no clear way to resolve the issue. Even then, you need to fix it after every code generation. For folks that have CI generate their code (to be sure not to have forgotten it) this will break their build by overwriting manual changes.

Anyway, I have the following solutions as input to the discussion:

  1. Add conflict checking, and output a warning/error so the user knows about the source of the conflict and how to resolve it (e.g. using an alias).
  2. After conflict checking, align the generated type names with how Apollo Android handles it (i.e. adding numbers to the end).
  3. After conflict checking, build a type name that is more readable and gives more context. Coming back to my example above, the type for values could be named something like ValuesValue. You wouldn't even need to touch the first one, but maybe it's easier to also rename the type for value to ValueValue. Not perfect but nicer.

@calvincestari calvincestari self-assigned this Mar 3, 2023
@calvincestari calvincestari added the planned-next Slated to be included in the next release label Mar 3, 2023
@calvincestari calvincestari added this to the Patch Releases (1.0.x) milestone Mar 3, 2023
@calvincestari
Copy link
Member

@AnthonyMDev and I discussed this issue today and we came to the conclusion that using a GraphQL field alias is the preferred way to resolve this kind of issue; it places control with the user in shaping the outcome rather than an arbitrary number tacked onto the end of the conflicting type.

That said though, the field alias is really only useful if you know what the problem is and what's required to resolve it. So we'll be changing the codegen engine to detect this kind of collision and throw an error causing codegen to fail with a helpful error message about what to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Generally incorrect behavior codegen Issues related to or arising from code generation 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