From 7f032a455c23276b7148ada1f238574ea192ecb2 Mon Sep 17 00:00:00 2001 From: Guy Davenport Date: Wed, 26 Jun 2024 16:12:10 +1200 Subject: [PATCH] better validation for required fields --- .../core/brapischema/BrAPISchemaReader.java | 33 ++++-- .../core/graphql/GraphQLGenerator.java | 103 +++++++++++------- .../core/graphql/options/IdsOptions.java | 35 +++++- .../src/main/resources/graphql-options.yaml | 14 ++- .../BrAPI-Schema/BrAPI-Common/Attribute.json | 4 + .../BrAPI-Genotyping/AlleleMatrix.json | 4 +- .../BrAPI-Schema/BrAPI-Genotyping/Call.json | 6 +- .../GermplasmAttributeValue.json | 3 +- .../BrAPI-Germplasm/PedigreeNode.json | 14 +-- .../BrAPI-Phenotyping/OntologyReference.json | 3 +- 10 files changed, 141 insertions(+), 78 deletions(-) diff --git a/java/core/src/main/java/org/brapi/schematools/core/brapischema/BrAPISchemaReader.java b/java/core/src/main/java/org/brapi/schematools/core/brapischema/BrAPISchemaReader.java index 60d8f51..2349437 100644 --- a/java/core/src/main/java/org/brapi/schematools/core/brapischema/BrAPISchemaReader.java +++ b/java/core/src/main/java/org/brapi/schematools/core/brapischema/BrAPISchemaReader.java @@ -440,21 +440,32 @@ private Response createObjectType(Path path, JsonNode jsonNode, Strin List required = findStringList(jsonNode, "required", false).getResultIfPresentOrElseResult(Collections.emptyList()); List properties = new ArrayList<>(); - return Response.empty(). - mapOnCondition(jsonNode.has("additionalProperties"), () -> findChildNode(jsonNode, "additionalProperties", true). + return Response.empty() + .mapOnCondition(jsonNode.has("additionalProperties"), () -> findChildNode(jsonNode, "additionalProperties", true). mapResultToResponse(additionalPropertiesNode -> createProperty(path, additionalPropertiesNode, "additionalProperties", - module, required.contains("additionalProperties")).onSuccessDoWithResult(properties::add))). - mapOnCondition(jsonNode.has("properties"), () -> findChildNode(jsonNode, "properties", true). - mapResult(JsonNode::fields). - mapResultToResponse(fields -> createProperties(path, fields, module, required)). - onSuccessDoWithResult(properties::addAll)). - onSuccessDo(() -> builder.properties(properties)). - mapOnCondition(jsonNode.has("brapi-metadata"), () -> findChildNode(jsonNode, "brapi-metadata", true). - mapResultToResponse(this::parseMetadata).onSuccessDoWithResult(builder::metadata)). - map(() -> success(builder.build())); + module, required.contains("additionalProperties")).onSuccessDoWithResult(properties::add))) + .mapOnCondition(jsonNode.has("properties"), () -> findChildNode(jsonNode, "properties", true) + .mapResult(JsonNode::fields) + .mapResultToResponse(fields -> createProperties(path, fields, module, required)) + .onSuccessDoWithResult(properties::addAll)) + .onSuccessDo(() -> builder.properties(properties)) + .merge(validateRequiredProperties(required, properties, name)) + .mapOnCondition(jsonNode.has("brapi-metadata"), () -> findChildNode(jsonNode, "brapi-metadata", true) + .mapResultToResponse(this::parseMetadata).onSuccessDoWithResult(builder::metadata)) + .map(() -> success(builder.build())); } + private Response> validateRequiredProperties(List requiredPropertyNames, List properties, String objectName) { + return requiredPropertyNames.stream().map(name -> validateRequiredProperty(name, properties, objectName)).collect(Response.toList()); + } + private Response validateRequiredProperty(String requiredPropertyName, List properties, String objectName) { + return properties.stream().filter(property -> property.getName().equals(requiredPropertyName)) + .findAny().map(Response::success) + .orElse(fail(Response.ErrorType.VALIDATION, + String.format("The required property '%s' is not found in the list of properties of '%s', expecting one of '%s", requiredPropertyName, objectName, + properties.stream().map(BrAPIObjectProperty::getName).collect(Collectors.joining(", "))))) ; + } private Response> createProperties(Path path, Iterator> fields, String module, List required) { return Streams.stream(fields).map(field -> createProperty(path, field.getValue(), field.getKey(), module, required.contains(field.getKey()))).collect(Response.toList()); diff --git a/java/core/src/main/java/org/brapi/schematools/core/graphql/GraphQLGenerator.java b/java/core/src/main/java/org/brapi/schematools/core/graphql/GraphQLGenerator.java index 3132d61..b7305a3 100644 --- a/java/core/src/main/java/org/brapi/schematools/core/graphql/GraphQLGenerator.java +++ b/java/core/src/main/java/org/brapi/schematools/core/graphql/GraphQLGenerator.java @@ -3,6 +3,7 @@ import graphql.AssertException; import graphql.TypeResolutionEnvironment; import graphql.schema.*; +import io.swagger.v3.oas.models.media.ObjectSchema; import lombok.AllArgsConstructor; import lombok.Getter; import org.brapi.schematools.core.brapischema.BrAPISchemaReader; @@ -190,39 +191,6 @@ private boolean isInterface(BrAPIClass type) { private Response createSchema(List primaryTypes) { GraphQLSchema.Builder builder = GraphQLSchema.newSchema(); - if (options.isGeneratingQueryType()) { - GraphQLObjectType.Builder query = newObject().name(options.getQueryType().getName()); - - if (options.isGeneratingSingleQueries()) { - primaryTypes.stream().filter(type -> options.getQueryType().getSingleQuery().isGeneratingFor(type.getName())).map(this::generateSingleGraphQLQuery).forEach(query::field); - } - - if (options.isGeneratingListQueries()) { - primaryTypes.stream().filter(type -> options.getQueryType().getListQuery().isGeneratingFor(type.getName())).map(this::generateListGraphQLQuery).forEach(query::field); - } - - if (options.isGeneratingSearchQueries()) { - primaryTypes.stream().filter(type -> options.getQueryType().getSearchQuery().isGeneratingFor(type.getName())).map(this::generateSearchGraphQLQuery).forEach(query::field); - } - - builder.query(query); - } - - if (options.isGeneratingMutationType()) { - GraphQLObjectType.Builder mutation = newObject().name(options.getMutationType().getName()); - - primaryTypes.stream().filter(type -> options.isGeneratingCreateMutationFor(type.getName())) - .map(this::generateCreateGraphQLMutation).forEach(mutation::field); - - primaryTypes.stream().filter(type -> options.isGeneratingUpdateMutationFor(type.getName())) - .map(this::generateUpdateGraphQLMutation).forEach(mutation::field); - - primaryTypes.stream().filter(type -> options.isGeneratingDeleteMutationFor(type.getName())) - .map(this::generateDeleteGraphQLMutation).forEach(mutation::field); - - builder.mutation(mutation); - } - objectOutputTypes.values().forEach(builder::additionalType); interfaceTypes.values().forEach(builder::additionalType); enumTypes.values().forEach(builder::additionalType); @@ -240,12 +208,54 @@ private Response createSchema(List primaryType builder.codeRegistry(codeRegistry.build()); try { - return success(builder.build()); + return Response.empty() + .mapOnCondition(options.isGeneratingQueryType(), () -> generateQueryType(primaryTypes).onSuccessDoWithResult(builder::query)) + .mapOnCondition(options.isGeneratingMutationType(), () -> generateMutationType(primaryTypes).onSuccessDoWithResult(builder::mutation)) + .withResult(builder.build()) ; } catch (AssertException e) { return fail(Response.ErrorType.VALIDATION, e.getMessage()); } } + private Response generateQueryType(List primaryTypes) { + GraphQLObjectType.Builder query = newObject().name(options.getQueryType().getName()); + + if (options.isGeneratingSingleQueries()) { + primaryTypes.stream().filter(type -> options.getQueryType().getSingleQuery().isGeneratingFor(type.getName())).map(this::generateSingleGraphQLQuery).forEach(query::field); + } + + if (options.isGeneratingListQueries()) { + primaryTypes.stream().filter(type -> options.getQueryType().getListQuery().isGeneratingFor(type.getName())).map(this::generateListGraphQLQuery).forEach(query::field); + } + + if (options.isGeneratingSearchQueries()) { + primaryTypes.stream().filter(type -> options.getQueryType().getSearchQuery().isGeneratingFor(type.getName())).map(this::generateSearchGraphQLQuery).forEach(query::field); + } + + return success(query.build()) ; + } + + private Response generateMutationType(List primaryTypes) { + GraphQLObjectType.Builder mutation = newObject().name(options.getMutationType().getName()); + + primaryTypes.stream() + .filter(type -> options.isGeneratingCreateMutationFor(type.getName())) + .map(this::generateCreateGraphQLMutation) + .forEach(mutation::field); + + primaryTypes.stream() + .filter(type -> options.isGeneratingUpdateMutationFor(type.getName())) + .map(this::generateUpdateGraphQLMutation) + .forEach(mutation::field); + + return primaryTypes.stream() + .filter(type -> options.isGeneratingDeleteMutationFor(type.getName())) + .map(this::generateDeleteGraphQLMutation) + .collect(Response.toList()) + .withResult(mutation.build()); + + } + private Response createOutputType(BrAPIType type) { if (type instanceof BrAPIObjectType) { @@ -726,12 +736,12 @@ private GraphQLOutputType creatSearchResponse(String queryName, GraphQLObjectTyp } private GraphQLFieldDefinition.Builder generateCreateGraphQLMutation(GraphQLObjectType type) { - return GraphQLFieldDefinition.newFieldDefinition() + return newFieldDefinition() .name(options.getCreateMutationNameFor(type.getName())) .description(createCreateMutationDescription(type)) .arguments(createCreateMutationArguments(type)) .type(options.getMutationType().getCreateMutation().isMultiple() ? - GraphQLList.list(GraphQLTypeReference.typeRef(type.getName())) : GraphQLTypeReference.typeRef(type.getName())) ; + GraphQLList.list(GraphQLTypeReference.typeRef(type.getName())) : GraphQLTypeReference.typeRef(type.getName())); } private String createCreateMutationDescription(GraphQLObjectType type) { @@ -793,27 +803,36 @@ private List createUpdateMutationArguments(GraphQLObjectType ty return arguments; } - private GraphQLFieldDefinition.Builder generateDeleteGraphQLMutation(GraphQLObjectType type) { - return GraphQLFieldDefinition.newFieldDefinition() + private Response generateDeleteGraphQLMutation(GraphQLObjectType type) { + GraphQLFieldDefinition.Builder builder = newFieldDefinition() .name(options.getDeleteMutationNameFor(type.getName())) .description(createDeleteMutationDescription(type)) - .arguments(createDeleteMutationArguments(type)) .type(options.getMutationType().getDeleteMutation().isMultiple() ? GraphQLList.list(GraphQLTypeReference.typeRef(type.getName())) : GraphQLTypeReference.typeRef(type.getName())) ; + + return createDeleteMutationArguments(type) + .onSuccessDoWithResult(builder::arguments) + .withResult(builder); } private String createDeleteMutationDescription(GraphQLObjectType type) { return options.getMutationType().getDeleteMutation().getDescriptionFor(type.getName()); } - private List createDeleteMutationArguments(GraphQLObjectType type) { + private Response> createDeleteMutationArguments(GraphQLObjectType type) { List arguments = new ArrayList<>(); + String idField = options.getIds().getIDFieldFor(type.getName()) ; + + if (type.getFields().stream().noneMatch(field -> field.getName().equals(idField))) { + return fail(Response.ErrorType.VALIDATION, String.format("Can not find field '%s' in type '%s'", idField, type.getName())) ; + } + GraphQLScalarType idType = options.isUsingIDType() ? GraphQLID : GraphQLString ; if (options.getMutationType().getDeleteMutation().isMultiple()) { arguments.add(GraphQLArgument.newArgument(). - name(toPlural(options.getIds().getNameFor(type.getName()))). + name(toPlural(idField)). type(GraphQLList.list(idType)). build()); } else { @@ -830,7 +849,7 @@ private List createDeleteMutationArguments(GraphQLObjectType ty build()); } - return arguments; + return success(arguments) ; } } diff --git a/java/core/src/main/java/org/brapi/schematools/core/graphql/options/IdsOptions.java b/java/core/src/main/java/org/brapi/schematools/core/graphql/options/IdsOptions.java index 8212dbc..b912ab7 100644 --- a/java/core/src/main/java/org/brapi/schematools/core/graphql/options/IdsOptions.java +++ b/java/core/src/main/java/org/brapi/schematools/core/graphql/options/IdsOptions.java @@ -6,6 +6,11 @@ import org.brapi.schematools.core.model.BrAPIType; import org.brapi.schematools.core.utils.StringUtils; +import java.util.HashMap; +import java.util.Map; + +import static org.brapi.schematools.core.utils.StringUtils.toParameterCase; + /** * Provides options for the generation of Ids */ @@ -18,9 +23,12 @@ public class IdsOptions { String nameFormat; @JsonProperty("useIDType") boolean usingIDType; + @Setter(AccessLevel.PRIVATE) + private Map fieldFor = new HashMap<>(); public void validate() { - assert nameFormat != null : "'nameFormat' option on Mutation Ids Options is null"; + assert nameFormat != null : String.format("'nameFormat' option on %s is null", this.getClass().getSimpleName()); + assert fieldFor != null : String.format("'fieldFor' option on %s is null", this.getClass().getSimpleName()); } /** @@ -42,4 +50,29 @@ public final String getNameFor(@NonNull String name) { public final String getNameFor(@NonNull BrAPIType type) { return getNameFor(type.getName()); } + + /** + * Gets the id field name for a specific primary model. For example the id field + * name of Study, would be 'studyDbiId' by default. Use {@link #setIDParameterFor} to override this value. + * @param name the name of the primary model + * @return id parameter name for a specific primary model + */ + @JsonIgnore + public String getIDFieldFor(String name) { + return fieldFor.getOrDefault(name, String.format(nameFormat, toParameterCase(name))) ; + } + + /** + * Sets the id field name for a specific primary model. For example the id field + * name of Study, would be 'studyDbiId' by default. + * @param name the name of the primary model + * @param idField the id field name for a specific primary model. + * @return the options for chaining + */ + @JsonIgnore + public IdsOptions setIDFieldFor(String name, String idField) { + fieldFor.put(name, idField) ; + + return this ; + } } diff --git a/java/core/src/main/resources/graphql-options.yaml b/java/core/src/main/resources/graphql-options.yaml index 0e5d914..cdb3d77 100644 --- a/java/core/src/main/resources/graphql-options.yaml +++ b/java/core/src/main/resources/graphql-options.yaml @@ -1,5 +1,11 @@ pluralFor: AlleleMatrix: AlleleMatrix +ids: + useIDType: true + nameFormat: "%sDbId" + fieldFor: + GermplasmAttribute: attributeDbId + GermplasmAttributeValue: attributeValueDbId input: name: input nameFormat: "%sInput" @@ -69,6 +75,7 @@ mutationType: Event: false GenomeMap: false MarkerPosition: false + PedigreeNode: false Reference: false Variant: false updateMutation: @@ -84,6 +91,7 @@ mutationType: Event: false GenomeMap: false MarkerPosition: false + PedigreeNode: false Reference: false Variant: false deleteMutation: @@ -99,9 +107,7 @@ mutationType: Event: false GenomeMap: false MarkerPosition: false + PedigreeNode: false Reference: false Variant: false - VariantSet: false -ids: - useIDType: true - nameFormat: "%sDbId" \ No newline at end of file + VariantSet: false \ No newline at end of file diff --git a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Common/Attribute.json b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Common/Attribute.json index 97a5b88..59a6c5a 100644 --- a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Common/Attribute.json +++ b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Common/Attribute.json @@ -13,6 +13,10 @@ "type": "string", "example": "Morphological" }, + "attributeDbId": { + "description": "The ID which uniquely identifies this attribute within the given database server", + "type": "string" + }, "attributeName": { "description": "A human readable name for this attribute", "type": "string", diff --git a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/AlleleMatrix.json b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/AlleleMatrix.json index c047122..4d3cd90 100644 --- a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/AlleleMatrix.json +++ b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/AlleleMatrix.json @@ -169,8 +169,8 @@ } }, "required": [ - "callSetDbIds", - "variantSetDbIds" + "callSets", + "variantSets" ], "title": "AlleleMatrix", "type": "object", diff --git a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/Call.json b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/Call.json index 3d9ab6b..3ce9d9e 100644 --- a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/Call.json +++ b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Genotyping/Call.json @@ -87,9 +87,9 @@ } }, "required": [ - "callSetDbId", - "variantDbId", - "variantSetDbId" + "callSet", + "variant", + "variantSet" ], "title": "Call", "type": "object", diff --git a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/GermplasmAttributeValue.json b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/GermplasmAttributeValue.json index 8fb064c..c16fb32 100644 --- a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/GermplasmAttributeValue.json +++ b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/GermplasmAttributeValue.json @@ -50,8 +50,7 @@ } }, "required": [ - "attributeValueDbId", - "attributeName" + "attribute" ], "title": "GermplasmAttributeValue", "type": "object", diff --git a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/PedigreeNode.json b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/PedigreeNode.json index 3fbcad8..c4fb7cb 100644 --- a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/PedigreeNode.json +++ b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Germplasm/PedigreeNode.json @@ -56,13 +56,6 @@ "referencedAttribute": "pedigreeNode", "relationshipType": "one-to-one" }, - "germplasmPUI": { - "description": "The Permanent Unique Identifier which represents a germplasm\n\nMIAPPE V1.1 (DM-41) Biological material ID - Code used to identify the biological material in the data file. Should be unique within the Investigation. Can correspond to experimental plant ID, seed lot ID, etc This material identification is different from a BiosampleID which corresponds to Observation Unit or Samples sections below.\n\nMIAPPE V1.1 (DM-51) Material source DOI - Digital Object Identifier (DOI) of the material source\n\nMCPD (v2.1) (PUID) 0. Any persistent, unique identifier assigned to the accession so it can be unambiguously referenced at the global level and the information associated with it harvested through automated means. Report one PUID for each accession. The Secretariat of the International Treaty on Plant Genetic Resources for Food and Agriculture (PGRFA) is facilitating the assignment of a persistent unique identifier (PUID), in the form of a DOI, to PGRFA at the accession level. Genebanks not applying a true PUID to their accessions should use, and request recipients to use, the concatenation of INSTCODE, ACCENUMB, and GENUS as a globally unique identifier similar in most respects to the PUID whenever they exchange information on accessions with third parties.", - "type": [ - "null", - "string" - ] - }, "parents": { "description": "A list of parent germplasm references in the pedigree tree for this germplasm. These represent edges in the tree, connecting to other nodes.\n
Typically, this array should only have one parent (clonal or self) or two parents (cross). In some special cases, there may be more parents, usually when the exact parent is not known. \n
If the parameter 'includeParents' is set to false, then this array should be empty, null, or not present in the response.", "items": { @@ -87,7 +80,7 @@ } }, "required": [ - "germplasmDbId", + "parentGermplasm", "parentType" ], "type": "object" @@ -128,7 +121,7 @@ } }, "required": [ - "germplasmDbId", + "progenyGermplasm", "parentType" ], "type": "object" @@ -153,8 +146,7 @@ } }, "required": [ - "germplasmDbId", - "germplasmName" + "germplasm" ], "title": "PedigreeNode", "type": "object", diff --git a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Phenotyping/OntologyReference.json b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Phenotyping/OntologyReference.json index 575aab6..0fea9eb 100644 --- a/java/core/src/test/resources/BrAPI-Schema/BrAPI-Phenotyping/OntologyReference.json +++ b/java/core/src/test/resources/BrAPI-Schema/BrAPI-Phenotyping/OntologyReference.json @@ -46,8 +46,7 @@ } }, "required": [ - "ontologyName", - "ontologyDbId" + "ontology" ], "title": "OntologyReference", "type": "object",