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

Fix generation of nested structures with the same name results in compiler errors #191

Merged
merged 4 commits into from
Feb 15, 2017
Merged

Fix generation of nested structures with the same name results in compiler errors #191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5215bdf
Fix generation of nested structures with the same name results in com…
sav007 Feb 13, 2017
d6c131d
Update unique name to have numeric suffixes
sav007 Feb 15, 2017
7b51d70
Merge remote-tracking branch 'upstream/master' into bug-169/unique-ty…
sav007 Feb 15, 2017
4ba104c
Fix
sav007 Feb 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...o-compiler/src/main/kotlin/com/apollographql/android/compiler/OperationTypeSpecBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,16 @@ class OperationTypeSpecBuilder(
private val OPERATION_VARIABLES_CLASS_NAME = ClassName.get("", "$OPERATION_TYPE_NAME.Variables")

override fun toTypeSpec(context: CodeGenerationContext): TypeSpec {
val newContext = context.plusReservedTypes(OPERATION_TYPE_NAME)
return TypeSpec.classBuilder(OPERATION_TYPE_NAME)
.addAnnotation(Annotations.GENERATED_BY_APOLLO)
.addModifiers(Modifier.PUBLIC, Modifier.FINAL)
.addQuerySuperInterface(operation.variables.isNotEmpty())
.addOperationDefinition(operation)
.addQueryDocumentDefinition(fragments, context)
.addQueryDocumentDefinition(fragments, newContext)
.addQueryConstructor(operation.variables.isNotEmpty())
.addVariablesDefinition(operation.variables, context.typesPackage, context.customTypeMap)
.addType(operation.toTypeSpec(context))
.addVariablesDefinition(operation.variables, newContext.typesPackage, newContext.customTypeMap)
.addType(operation.toTypeSpec(newContext))
.build()
}

Expand Down
10 changes: 3 additions & 7 deletions apollo-compiler/src/main/kotlin/com/apollographql/android/compiler/SchemaTypeSpecBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,16 @@ class SchemaTypeSpecBuilder(
private fun TypeSpec.Builder.addInnerTypes(fields: List<Field>): TypeSpec.Builder {
val reservedTypeNames = context.reservedTypeNames + typeName + fields.filter(Field::isNonScalar).map(
Field::normalizedName)
val typeSpecs = fields.filter(Field::isNonScalar).map {
it.toTypeSpec(CodeGenerationContext(context.abstractType, reservedTypeNames.minus(it.normalizedName()),
context.typeDeclarations, context.fragmentsPackage, context.typesPackage, context.customTypeMap))
val typeSpecs = fields.filter(Field::isNonScalar).map { field ->
field.toTypeSpec(context.withReservedTypeNames(reservedTypeNames.minus(field.normalizedName())))
}
return addTypes(typeSpecs)
}

private fun TypeSpec.Builder.addInlineFragments(fragments: List<InlineFragment>): TypeSpec.Builder {
val reservedTypeNames = context.reservedTypeNames + typeName + fields.filter(Field::isNonScalar).map(
Field::normalizedName)
val typeSpecs = fragments.map {
it.toTypeSpec(CodeGenerationContext(context.abstractType, reservedTypeNames, context.typeDeclarations,
context.fragmentsPackage, context.typesPackage, context.customTypeMap))
}
val typeSpecs = fragments.map { it.toTypeSpec(context.withReservedTypeNames(reservedTypeNames)) }
val methodSpecs = fragments.map { it.accessorMethodSpec(context.abstractType) }
val fieldSpecs = if (context.abstractType) emptyList() else fragments.map { it.fieldSpec() }
return addTypes(typeSpecs)
Expand Down
26 changes: 25 additions & 1 deletion ...o-compiler/src/main/kotlin/com/apollographql/android/compiler/ir/CodeGenerationContext.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,28 @@ data class CodeGenerationContext(
val fragmentsPackage: String = "",
val typesPackage: String = "",
val customTypeMap: Map<String, String>
)
) {
fun plusReservedTypes(vararg typeName: String): CodeGenerationContext = plusReservedTypes(typeName.toList())

fun plusReservedTypes(typeNames: List<String>): CodeGenerationContext =
CodeGenerationContext(
abstractType = abstractType,
reservedTypeNames = reservedTypeNames.plus(typeNames),
typeDeclarations = typeDeclarations,
fragmentsPackage = fragmentsPackage,
typesPackage = typesPackage,
customTypeMap = customTypeMap
)

fun withReservedTypeNames(vararg typeName: String): CodeGenerationContext = withReservedTypeNames(typeName.asList())

fun withReservedTypeNames(reservedTypeNames: List<String>): CodeGenerationContext =
CodeGenerationContext(
abstractType = abstractType,
reservedTypeNames = reservedTypeNames,
typeDeclarations = typeDeclarations,
fragmentsPackage = fragmentsPackage,
typesPackage = typesPackage,
customTypeMap = customTypeMap
)
}
3 changes: 1 addition & 2 deletions apollo-compiler/src/main/kotlin/com/apollographql/android/compiler/ir/Operation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.apollographql.android.compiler.ir

import com.apollographql.android.api.graphql.Operation
import com.apollographql.android.compiler.SchemaTypeSpecBuilder
import com.squareup.javapoet.ClassName
import com.squareup.javapoet.TypeSpec
import javax.lang.model.element.Modifier

Expand All @@ -19,7 +18,7 @@ data class Operation(
SchemaTypeSpecBuilder(INTERFACE_TYPE_SPEC_NAME, fields, emptyList(), emptyList(), context)
.build(Modifier.PUBLIC, Modifier.STATIC)
.toBuilder()
.addSuperinterface(ClassName.get(Operation.Data::class.java))
.addSuperinterface(Operation.Data::class.java)
.build()

companion object {
Expand Down
36 changes: 18 additions & 18 deletions ...e/unique_type_name/TestQueryExpected.java → ...ue_type_name/HeroDetailQueryExpected.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
import javax.annotation.Nullable;

@Generated("Apollo GraphQL")
public final class TestQuery implements Query<Operation.Variables> {
public static final String OPERATION_DEFINITION = "query TestQuery {\n"
+ " hero {\n"
public final class HeroDetailQuery implements Query<Operation.Variables> {
public static final String OPERATION_DEFINITION = "query HeroDetailQuery {\n"
+ " heroDetailQuery {\n"
+ " __typename\n"
+ " name\n"
+ " friends {\n"
Expand All @@ -46,7 +46,7 @@ public final class TestQuery implements Query<Operation.Variables> {

private final Operation.Variables variables;

public TestQuery() {
public HeroDetailQuery() {
this.variables = Operation.EMPTY_VARIABLES;
}

Expand All @@ -61,9 +61,9 @@ public Operation.Variables variables() {
}

public interface Data extends Operation.Data {
@Nullable Hero hero();
@Nullable HeroDetailQuery$ heroDetailQuery();
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if nested twice? tbh I'd prefer HeroDetailQuery$$1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be HeroDetailQuery$$

Copy link
Contributor

Choose a reason for hiding this comment

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

And just for clarity 5 would be Query$$$$$ so just as example imagine you see following in logs...
NullPointerExxeption: Foo$$$$ is null

Will you be able to tell without dragging your finger in screen whether that was 4 or 5 dollar signs.
I don't feel strongly about it but can see it leading to confusing traces. As example rxjava does Action0 - Action9

Copy link
Contributor Author

@sav007 sav007 Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure why we initially decided to go with $, but it's really easy to change in compiler. But the idea is that in most cases you won't have such issue with more than 2-3 nested classes with the same name. But I don't have any strong opinion for this as Hero4 and Hero$$$$ from my perspective is the same evil, and for sure I will try to avoid such queries with the same nested names (will use aliases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@digitalbuddha so are we changing $ to number or leaving as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer number but you make the call :-)


interface Hero {
interface HeroDetailQuery$ {
@Nonnull String name();

@Nullable List<? extends Friend> friends();
Expand Down Expand Up @@ -347,7 +347,7 @@ interface Creator {
}
}

final class Mapper implements ResponseFieldMapper<Hero> {
final class Mapper implements ResponseFieldMapper<HeroDetailQuery$> {
final Factory factory;

final Field[] fields = {
Expand All @@ -374,7 +374,7 @@ public Mapper(@Nonnull Factory factory) {
}

@Override
public Hero map(ResponseReader reader) throws IOException {
public HeroDetailQuery$ map(ResponseReader reader) throws IOException {
final __ContentValues contentValues = new __ContentValues();
reader.toBufferedReader().read(new ResponseReader.ValueHandler() {
@Override
Expand Down Expand Up @@ -416,18 +416,18 @@ interface Factory {
}

interface Creator {
@Nonnull Hero create(@Nonnull String name, @Nullable List<? extends Friend> friends,
@Nullable AsHuman asHuman);
@Nonnull HeroDetailQuery$ create(@Nonnull String name,
@Nullable List<? extends Friend> friends, @Nullable AsHuman asHuman);
}
}

final class Mapper implements ResponseFieldMapper<Data> {
final Factory factory;

final Field[] fields = {
Field.forObject("hero", "hero", null, true, new Field.ObjectReader<Hero>() {
@Override public Hero read(final ResponseReader reader) throws IOException {
return new Hero.Mapper(factory.heroFactory()).map(reader);
Field.forObject("heroDetailQuery", "heroDetail", null, true, new Field.ObjectReader<HeroDetailQuery$>() {
@Override public HeroDetailQuery$ read(final ResponseReader reader) throws IOException {
return new HeroDetailQuery$.Mapper(factory.heroDetailQuery$Factory()).map(reader);
}
})
};
Expand All @@ -444,28 +444,28 @@ public Data map(ResponseReader reader) throws IOException {
public void handle(final int fieldIndex, final Object value) throws IOException {
switch (fieldIndex) {
case 0: {
contentValues.hero = (Hero) value;
contentValues.heroDetailQuery = (HeroDetailQuery$) value;
break;
}
}
}
}, fields);
return factory.creator().create(contentValues.hero);
return factory.creator().create(contentValues.heroDetailQuery);
}

static final class __ContentValues {
Hero hero;
HeroDetailQuery$ heroDetailQuery;
}
}

interface Factory {
@Nonnull Creator creator();

@Nonnull Hero.Factory heroFactory();
@Nonnull HeroDetailQuery$.Factory heroDetailQuery$Factory();
}

interface Creator {
@Nonnull Data create(@Nullable Hero hero);
@Nonnull Data create(@Nullable HeroDetailQuery$ heroDetailQuery);
}
}
}
8 changes: 4 additions & 4 deletions apollo-compiler/src/test/graphql/com/example/unique_type_name/TestQuery.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
{
"operations": [
{
"operationName": "TestQuery",
"operationName": "HeroDetailQuery",
"operationType": "query",
"variables": [],
"source": "query TestQuery {\n hero {\n __typename\n name\n friends {\n __typename\n name\n }\n ... on Human {\n height\n friends {\n __typename\n appearsIn\n friends {\n __typename\n ...HeroDetails\n }\n }\n }\n }\n}",
"source": "query HeroDetailQuery {\n heroDetailQuery {\n __typename\n name\n friends {\n __typename\n name\n }\n ... on Human {\n height\n friends {\n __typename\n appearsIn\n friends {\n __typename\n ...HeroDetails\n }\n }\n }\n }\n}",
"fields": [
{
"responseName": "hero",
"fieldName": "hero",
"responseName": "heroDetailQuery",
"fieldName": "heroDetail",
"type": "Character",
"fields": [
{
Expand Down