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

Handle duplicate fields from the type and interface #128 #129

Merged
merged 4 commits into from
Apr 29, 2020
Merged

Handle duplicate fields from the type and interface #128 #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e3f3e4
Handle duplicate fields from the type and interface #128
kobylynskyi Apr 28, 2020
3bd4bf0
Take annotations and javadoc from the type and not the interface #128
kobylynskyi Apr 29, 2020
0507780
Refactor and add some unit tests
kobylynskyi Apr 29, 2020
0ada7b7
Simplify the code in TypeDefinitionToDataModelMapper #130
kobylynskyi Apr 29, 2020
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
73 changes: 45 additions & 28 deletions src/main/java/com/kobylynskyi/graphql/codegen/mapper/TypeDefinitionToDataModelMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,22 +80,39 @@ public static Map<String, Object> mapResponseProjection(MappingConfig mappingCon
* @param document Parent GraphQL document
* @return Freemarker data model of the GraphQL type
*/
private static Set<ParameterDefinition> getFields(MappingConfig mappingConfig,
ExtendedObjectTypeDefinition typeDefinition,
ExtendedDocument document) {
// this includes parameters from base definition and extensions
List<ParameterDefinition> typeParameters = FieldDefinitionToParameterMapper.mapFields(mappingConfig,
typeDefinition.getFieldDefinitions(), typeDefinition.getName());
List<ParameterDefinition> typeParametersFromInterfaces = getInterfacesOfType(typeDefinition, document)
.stream()
private static Collection<ParameterDefinition> getFields(MappingConfig mappingConfig,
ExtendedObjectTypeDefinition typeDefinition,
ExtendedDocument document) {
// using the map to exclude duplicate fields from the type and interfaces
Map<String, ParameterDefinition> allParameters = new LinkedHashMap<>();

// includes parameters from the base definition and extensions
FieldDefinitionToParameterMapper.mapFields(mappingConfig, typeDefinition.getFieldDefinitions(), typeDefinition.getName())
.forEach(p -> allParameters.put(p.getName(), p));
// includes parameters from the interface
getInterfacesOfType(typeDefinition, document).stream()
Copy link
Contributor

@joffrey-bion joffrey-bion Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to get fields from the interface in the first place?
By definition, for any implemented interface, all fields should be declared in the type itself as well.

Is it to get metadata like Javadoc?
In this case I think we should "merge" the interface field with the corresponding type's field, because I imagine the doc or directives can also come from the type's field.
Maybe we should take everything from the type's field in priority, and if that field doesn't have a metadata element (like a doc) we fallback to the metadata from the interface's field.

Copy link
Owner Author

@kobylynskyi kobylynskyi Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

From the example that is shown in the bug, javadoc was present in the interface and not the type.
Another example: you can require custom serialization for a data type that is defined in the interface. And you don’t want to duplicate directives and javadoc in all types for this field. So you will define it in the interface. The type will have just the field.

Copy link
Contributor

@joffrey-bion joffrey-bion Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's why we need to read the interface's fields as well, indeed.
However, we could imagine a situation where the doc is overridden in the type (or not even present in the interface), and in that case we need to take the doc from the type's field, not the interface's one.

But with the code as it is here, we would simply override the type's fields data with the interface's one, and lose anything declared directly in the implementing type.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've modified the logic to take JavaDoc and annotations from the type.
And if something is absent then it will be taken from the interface field.

Copy link
Contributor

@joffrey-bion joffrey-bion Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm so sorry to bring this up only now, but now I'm wondering whether we need to look at interface fields at all.
I mean, to generate the Java code for the interface, we use the interface fields and docs. And to generate the Java code for the implementing class, we don't need to look at the interface fields at all, do we?
Javadoc would naturally be inherited by overriding methods (we don't even need to do this at codegen level). And annotations are not inherited, but do we want that?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Could you please create a separate issue with your suggestion?
We can discuss it there.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure I can do that, I just felt like it was another way to fix the issue at hand, not a separate issue.
But you're right, we can merge this and fix the bug right away.

.map(i -> FieldDefinitionToParameterMapper.mapFields(mappingConfig, i.getFieldDefinitions(), i.getName()))
.flatMap(Collection::stream)
.collect(Collectors.toList());
.forEach(paramDef -> allParameters.merge(paramDef.getName(), paramDef, TypeDefinitionToDataModelMapper::merge));
return allParameters.values();
}

Set<ParameterDefinition> allParameters = new LinkedHashSet<>();
allParameters.addAll(typeParameters);
allParameters.addAll(typeParametersFromInterfaces);
return allParameters;
/**
* Merge parameter definition data from the type and interface
* Annotations from the type have higher precedence
*
* @param typeDef Definition of the same parameter from the type
* @param interfaceDef Definition of the same parameter from the interface
* @return merged parameter definition
*/
private static ParameterDefinition merge(ParameterDefinition typeDef, ParameterDefinition interfaceDef) {
if (Utils.isEmpty(typeDef.getAnnotations())) {
typeDef.setAnnotations(interfaceDef.getAnnotations());
}
if (Utils.isEmpty(typeDef.getJavaDoc())) {
typeDef.setJavaDoc(interfaceDef.getJavaDoc());
}
return typeDef;
}

/**
Expand All @@ -107,23 +124,23 @@ private static Set<ParameterDefinition> getFields(MappingConfig mappingConfig,
* @param typeNames Names of all GraphQL types
* @return Freemarker data model of the GraphQL type
*/
private static Set<ProjectionParameterDefinition> getProjectionFields(MappingConfig mappingConfig,
ExtendedObjectTypeDefinition typeDefinition,
ExtendedDocument document,
Set<String> typeNames) {
// this includes parameters from base definition and extensions
List<ProjectionParameterDefinition> typeParameters = FieldDefinitionToParameterMapper.mapProjectionFields(
mappingConfig, typeDefinition.getFieldDefinitions(), typeNames);
List<ProjectionParameterDefinition> typeParametersFromInterfaces = getInterfacesOfType(typeDefinition, document)
.stream()
private static Collection<ProjectionParameterDefinition> getProjectionFields(MappingConfig mappingConfig,
ExtendedObjectTypeDefinition typeDefinition,
ExtendedDocument document,
Set<String> typeNames) {
// using the map to exclude duplicate fields from the type and interfaces
Map<String, ProjectionParameterDefinition> allParameters = new LinkedHashMap<>();

// includes parameters from the base definition and extensions
FieldDefinitionToParameterMapper.mapProjectionFields(mappingConfig, typeDefinition.getFieldDefinitions(), typeNames)
.forEach(p -> allParameters.put(p.getName(), p));
// includes parameters from the interface
getInterfacesOfType(typeDefinition, document).stream()
.map(i -> FieldDefinitionToParameterMapper.mapProjectionFields(mappingConfig, i.getFieldDefinitions(), typeNames))
.flatMap(Collection::stream)
.collect(Collectors.toList());

Set<ProjectionParameterDefinition> allParameters = new LinkedHashSet<>();
allParameters.addAll(typeParameters);
allParameters.addAll(typeParametersFromInterfaces);
return allParameters;
.filter(paramDef -> !allParameters.containsKey(paramDef.getName()))
.forEach(paramDef -> allParameters.put(paramDef.getName(), paramDef));
return allParameters.values();
}

/**
Expand Down
25 changes: 12 additions & 13 deletions src/test/java/com/kobylynskyi/graphql/codegen/GraphQLCodegenDefaultsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,27 @@
package com.kobylynskyi.graphql.codegen;

import com.kobylynskyi.graphql.codegen.model.MappingConfig;
import com.kobylynskyi.graphql.codegen.utils.Utils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Objects;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import com.kobylynskyi.graphql.codegen.model.MappingConfig;
import com.kobylynskyi.graphql.codegen.utils.Utils;

import static com.kobylynskyi.graphql.codegen.TestUtils.assertSameTrimmedContent;
import static java.util.stream.Collectors.toList;
import static org.junit.jupiter.api.Assertions.assertEquals;

class GraphQLCodegenDefaultsTest {

private GraphQLCodegen generator;

private File outputBuildDir = new File("build/generated");
private File outputJavaClassesDir = new File("build/generated/com/kobylynskyi/graphql/testdefaults");
private final File outputBuildDir = new File("build/generated");
private final File outputJavaClassesDir = new File("build/generated/com/kobylynskyi/graphql/testdefaults");

@BeforeEach
void init() {
Expand All @@ -33,7 +32,7 @@ void init() {
}

@AfterEach
void cleanup() throws IOException {
void cleanup() {
Utils.deleteDir(new File("build/generated"));
}

Expand All @@ -46,8 +45,8 @@ void generate_CheckFiles() throws Exception {
assertEquals(Arrays.asList("InputWithDefaults.java", "MyEnum.java", "SomeObject.java"), generatedFileNames);

for (File file : files) {
File expected = new File(String.format("src/test/resources/expected-classes/%s.txt", file.getName()));
TestUtils.assertSameTrimmedContent(expected, file);
assertSameTrimmedContent(new File(String.format("src/test/resources/expected-classes/%s.txt", file.getName())),
file);
}
}
}
20 changes: 8 additions & 12 deletions src/test/java/com/kobylynskyi/graphql/codegen/GraphQLCodegenExtendTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@
import org.junit.jupiter.api.Test;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.Paths;
import java.util.*;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;

import static com.kobylynskyi.graphql.codegen.TestUtils.assertSameTrimmedContent;
import static com.kobylynskyi.graphql.codegen.TestUtils.getFileByName;
import static java.util.Arrays.asList;
import static java.util.stream.Collectors.toSet;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -31,7 +33,7 @@ void init() {
}

@AfterEach
void cleanup() throws IOException {
void cleanup() {
Utils.deleteDir(new File("build/generated"));
}

Expand Down Expand Up @@ -104,19 +106,13 @@ void generateClientSideClasses() throws Exception {

File[] files = Objects.requireNonNull(outputJavaClassesDir.listFiles());

File eventResponseProjectionFile = Arrays.stream(files)
.filter(file -> file.getName().equalsIgnoreCase("EventResponseProjection.java")).findFirst()
.orElseThrow(FileNotFoundException::new);
assertSameTrimmedContent(
new File("src/test/resources/expected-classes/extend/request/EventResponseProjection.java.txt"),
eventResponseProjectionFile);
getFileByName(files, "EventResponseProjection.java"));

File assetResponseProjectionFile = Arrays.stream(files)
.filter(file -> file.getName().equalsIgnoreCase("AssetResponseProjection.java")).findFirst()
.orElseThrow(FileNotFoundException::new);
assertSameTrimmedContent(
new File("src/test/resources/expected-classes/extend/request/AssetResponseProjection.java.txt"),
assetResponseProjectionFile);
getFileByName(files, "AssetResponseProjection.java"));
}

@Test
Expand Down
8 changes: 2 additions & 6 deletions src/test/java/com/kobylynskyi/graphql/codegen/GraphQLCodegenExternalConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,9 @@ class GraphQLCodegenExternalConfigTest {

/**
* Check mapping config from json file.
*
* @throws Exception the exception
*/
@Test
void check_mappingConfigFromJsonFile() throws Exception {
void check_mappingConfigFromJsonFile() {
MappingConfig externalMappingConfig = new JsonMappingConfigSupplier("src/test/resources/json/mappingconfig.json").get();

assertEquals(externalMappingConfig.getPackageName(), "com.kobylynskyi.graphql.testconfigjson");
Expand All @@ -32,11 +30,9 @@ void check_mappingConfigFromJsonFile() throws Exception {

/**
* Check combine mapping config with external.
*
* @throws Exception the exception
*/
@Test
void check_combineMappingConfigWithExternal() throws Exception {
void check_combineMappingConfigWithExternal() {
MappingConfig mappingConfig = new MappingConfig();
mappingConfig.setPackageName("com.kobylynskyi.graphql.test1");

Expand Down
3 changes: 1 addition & 2 deletions src/test/java/com/kobylynskyi/graphql/codegen/GraphQLCodegenFieldsResolversTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -33,7 +32,7 @@ void init() {
}

@AfterEach
void cleanup() throws IOException {
void cleanup() {
Utils.deleteDir(new File("build/generated"));
}

Expand Down
37 changes: 15 additions & 22 deletions src/test/java/com/kobylynskyi/graphql/codegen/GraphQLCodegenGitHubTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@
import org.junit.jupiter.api.Test;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.Objects;

import static com.kobylynskyi.graphql.codegen.TestUtils.assertSameTrimmedContent;
import static com.kobylynskyi.graphql.codegen.TestUtils.getFileByName;
import static org.hamcrest.MatcherAssert.assertThat;

class GraphQLCodegenGitHubTest {
Expand All @@ -34,7 +33,7 @@ void init() {
}

@AfterEach
void cleanup() throws IOException {
void cleanup() {
Utils.deleteDir(new File("build/generated"));
}

Expand All @@ -43,11 +42,12 @@ void generate_MultipleInterfacesPerType() throws Exception {
generator.generate();

File[] files = Objects.requireNonNull(outputJavaClassesDir.listFiles());
File commitFile = getGeneratedFile(files, "Commit.java");
assertSameTrimmedContent(new File("src/test/resources/expected-classes/Commit.java.txt"), commitFile);

File profileOwner = getGeneratedFile(files, "ProfileOwner.java");
assertSameTrimmedContent(new File("src/test/resources/expected-classes/ProfileOwner.java.txt"), profileOwner);
assertSameTrimmedContent(new File("src/test/resources/expected-classes/Commit.java.txt"),
getFileByName(files, "Commit.java"));

assertSameTrimmedContent(new File("src/test/resources/expected-classes/ProfileOwner.java.txt"),
getFileByName(files, "ProfileOwner.java"));
}

@Test
Expand All @@ -60,46 +60,39 @@ void generate_ClassNameWithSuffix_Prefix() throws Exception {
File[] files = Objects.requireNonNull(outputJavaClassesDir.listFiles());

// verify proper class name for GraphQL interface
assertThat(getGeneratedFileContent(files, "GithubActorTO.java"),
assertThat(getFileContent(files, "GithubActorTO.java"),
StringContains.containsString("public interface GithubActorTO "));

// verify proper class name for GraphQL enum
assertThat(getGeneratedFileContent(files, "GithubIssueStateTO.java"),
assertThat(getFileContent(files, "GithubIssueStateTO.java"),
StringContains.containsString("public enum GithubIssueStateTO "));

// verify proper class name for GraphQL union
assertThat(getGeneratedFileContent(files, "GithubAssigneeTO.java"),
assertThat(getFileContent(files, "GithubAssigneeTO.java"),
StringContains.containsString("public interface GithubAssigneeTO "));

// verify proper class name for GraphQL input
assertSameTrimmedContent(
new File("src/test/resources/expected-classes/GithubAcceptTopicSuggestionInputTO.java.txt"),
getGeneratedFile(files, "GithubAcceptTopicSuggestionInputTO.java"));
getFileByName(files, "GithubAcceptTopicSuggestionInputTO.java"));

// verify proper class name for GraphQL type and references to interfaces/types/unions for GraphQL type
assertSameTrimmedContent(new File("src/test/resources/expected-classes/GithubCommitTO.java.txt"),
getGeneratedFile(files, "GithubCommitTO.java"));
getFileByName(files, "GithubCommitTO.java"));
}

@Test
void generate_NoValidationAnnotation() throws Exception {
mappingConfig.setModelValidationAnnotation(null);

generator.generate();
File commitFile = getGeneratedFile(Objects.requireNonNull(outputJavaClassesDir.listFiles()), "Commit.java");
File commitFile = getFileByName(Objects.requireNonNull(outputJavaClassesDir.listFiles()), "Commit.java");
assertSameTrimmedContent(new File("src/test/resources/expected-classes/Commit_noValidationAnnotation.java.txt"),
commitFile);
}

private static String getGeneratedFileContent(File[] files, String fileName) throws IOException {
File file = getGeneratedFile(files, fileName);
return Utils.getFileContent(file.getPath());
private static String getFileContent(File[] files, String fileName) throws IOException {
return Utils.getFileContent(getFileByName(files, fileName).getPath());
}

private static File getGeneratedFile(File[] files, String fileName) throws FileNotFoundException {
return Arrays.stream(files)
.filter(f -> f.getName().equalsIgnoreCase(fileName))
.findFirst()
.orElseThrow(FileNotFoundException::new);
}
}
28 changes: 14 additions & 14 deletions src/test/java/com/kobylynskyi/graphql/codegen/GraphQLCodegenMultiFilesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,26 @@
package com.kobylynskyi.graphql.codegen;

import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;

import com.kobylynskyi.graphql.codegen.model.MappingConfig;
import com.kobylynskyi.graphql.codegen.utils.Utils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import com.kobylynskyi.graphql.codegen.model.MappingConfig;
import com.kobylynskyi.graphql.codegen.utils.Utils;
import java.io.File;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;

import static com.kobylynskyi.graphql.codegen.TestUtils.assertSameTrimmedContent;
import static java.util.stream.Collectors.toList;
import static org.junit.jupiter.api.Assertions.assertEquals;

class GraphQLCodegenMultiFilesTest {

private GraphQLCodegen generator;
private final File outputBuildDir = new File("build/generated");
private final File outputJavaClassesDir = new File("build/generated/com/kobylynskyi/graphql/multifiles");

private File outputBuildDir = new File("build/generated");
private File outputJavaClassesDir = new File("build/generated/com/kobylynskyi/graphql/multifiles");
private GraphQLCodegen generator;

@BeforeEach
void init() {
Expand All @@ -35,7 +34,7 @@ void init() {
}

@AfterEach
void cleanup() throws IOException {
void cleanup() {
Utils.deleteDir(new File("build/generated"));
}

Expand All @@ -48,8 +47,9 @@ void generate_CheckFiles() throws Exception {
assertEquals(Arrays.asList("MyUnion.java", "UnionMember1.java", "UnionMember2.java"), generatedFileNames);

for (File file : files) {
File expected = new File(String.format("src/test/resources/expected-classes/%s.txt", file.getName()));
TestUtils.assertSameTrimmedContent(expected, file);
assertSameTrimmedContent(
new File(String.format("src/test/resources/expected-classes/%s.txt", file.getName())),
file);
}
}
}
Loading