Changed name to Species to OntologyTerm, removed DbIds not in 2.1 mod… by daveneti · Pull Request #616 · plantbreeding/BrAPI

daveneti · 2024-10-10T23:01:36Z

Changed name to Species to OntologyTerm, removed DbIds not in 2.1 model, matched ObservationUnitLevel etc with model.

…el, matched ObservationUnitLevel etc with model.

LzLang

just reviewed your changes and there are only small changes needed.

It would certainly be helpful to adjust the validator so that it checks the associations.
For example, you could automatically check whether a referencedAttribute is present and whether the type of the reverse association is correct.
(If the validator is not already doing this)

LzLang · 2024-10-17T11:32:17Z

Specification/BrAPI-Schema/BrAPI-Common/ExternalReference.json

        "ExternalReference": {
            "properties": {
-                "externalReferenceDbId": {
+                "referenceId": {


Wouldn't it make sense to use referenceDbId here so that it is standardized or would that lead to a model change?

It the OpenAPI specification it is 'referenceId' so we should keep it to that. Also I think it is technically not a DbId but can be any ID.

LzLang · 2024-10-17T11:38:48Z

Specification/BrAPI-Schema/BrAPI-Core/Study.json

@@ -455,9 +440,6 @@
                    "referencedAttribute": "growthFacility"


You changed the relationship in "Study" from "one-to-many" to "many-to-one".
Reverse association should be "one-to-many" than

A study has one growth facility, but a growth facility may have several studies so

study -> growth facility is many-to-one
growth facilities->study will be one-to-many, correct :)

LzLang · 2024-10-17T11:40:53Z

Specification/BrAPI-Schema/BrAPI-Core/Study.json

-                    "referencedAttribute": "study",
                    "items": {
-                        "$ref": "../BrAPI-Core/Study.json#/$defs/ObservationLevels",
+                        "$ref": "../BrAPI-Common/ObservationUnitHierarchyLevel.json#/$defs/ObservationUnitHierarchyLevel",


You have here a association to ObservationUnitHierarchyLevel but no referencedAttribute nor reverse association.
Is that correct and wanted?

I am not sure it is needed. It is not in the model. Actually many of the reverse associations are not in the model, we need to discuss with @BrapiCoordinatorSelby about what we should do about this.

LzLang · 2024-10-17T11:46:22Z

Specification/BrAPI-Schema/BrAPI-Genotyping/Reference.json

                    "referencedAttribute": "reference",
                    "items": {
-                        "$ref": "../BrAPI-Common/SourceGermplasm.json#/$defs/SourceGermplasm",
+                        "$ref": "../BrAPI-Germplasm/Germplasm.json#/$defs/Germplasm",


There is no reverse association, is that intentional?

See the above comment about reverse associations. If you need something like that in your GraphQL schema we could add an option to auto generate reverse associations. I still think reverse associations should be explicitly defined and model dependent. We should review them all.

LzLang · 2024-10-17T11:47:11Z

Specification/BrAPI-Schema/BrAPI-Genotyping/Reference.json

                    "relationshipType": "one-to-one",
                    "referencedAttribute": "reference",
-					"$ref": "../BrAPI-Common/Species.json#/$defs/Species"
+					"$ref": "../BrAPI-Common/OntologyTerm.json#/$defs/OntologyTerm"


See comment above

see my comment above :)

LzLang · 2024-10-17T12:00:01Z

Specification/BrAPI-Schema/BrAPI-Germplasm/SeedLot.json

+                "germplasm": {
+                    "$ref": "Germplasm.json#/$defs/Germplasm",
+                    "description": "The ID which uniquely identifies a germplasm",
+                    "referencedAttribute": "parentPedigreeNodes",


reverse association uses "$ref": "PedigreeNode.json#/$defs/PedigreeNode",

LzLang · 2024-10-17T12:01:19Z

Specification/BrAPI-Schema/BrAPI-Phenotyping/ObservationUnitLevelRelationship.json

+                    "properties": {
+                        "observationUnit": {
+                            "description": "The an observation unit\n<br/>If this level has on ObservationUnit associated with it, record the observationUnitDbId here. This is intended to construct a strict hierarchy of observationUnits. \n<br/>If there is no ObservationUnit associated with this level, this field can set to NULL or omitted from the response.",
+                            "$ref": "../BrAPI-Phenotyping/ObservationUnit.json#/$defs/ObservationUnit",


No referencedAttribute, is that intentional?

See comments about referenced attributes

LzLang · 2024-10-17T12:02:02Z

Specification/BrAPI-Schema/BrAPI-Phenotyping/ObservationUnitLevelRelationship.json

+                    }
                }
-            },
-            "title": "ObservationUnitLevelRelationship",


No title nor type for the model?

no type is needed because it is an 'allof' schema, but it could have a title, although it is option can defaults the to name. I did not put titles in any of the 'allof' schemas.

LzLang · 2024-10-17T12:03:50Z

Specification/BrAPI-Schema/BrAPI-Phenotyping/Scale.json

                },
                "validValues": {
-                    "relationshipType": "one-to-many",
+                    "relationshipType": "one-to-one",


reverse association is still many-to-one

I think it is 1 to 1, always. The 'ValidValues' entity is always associated with a Scale. It is a Composition association. It only exists with a single Scale object and no other.

LzLang · 2024-10-17T12:13:19Z

Specification/BrAPI-Schema/BrAPI-Genotyping/VariantSet.json

@@ -296,21 +308,14 @@
                    "referencedAttribute": "availableFormats"


relationshipType is still many-to-one, reverse is many-to-many

A single VarientSet can have multiple formats so one-to-many
A format can be shared between VarientSets, so many-to-many

daveneti · 2024-10-18T04:34:09Z

Hey @daveneti,

just reviewed your changes and there are only small changes needed.

It would certainly be helpful to adjust the validator so that it checks the associations. For example, you could automatically check whether a referencedAttribute is present and whether the type of the reverse association is correct. (If the validator is not already doing this)

@LzLang it should check references. I am also adding a CI so that the validator is run on commit. wait for @BrapiCoordinatorSelby permission.

Changed name to Species to OntologyTerm, removed DbIds not in 2.1 mod…

0bf932c

…el, matched ObservationUnitLevel etc with model.

daveneti requested review from BrapiCoordinatorSelby and LzLang October 10, 2024 23:01

LzLang mentioned this pull request Oct 15, 2024

Revert "One-dimensionalization of data models" #617

Closed

LzLang reviewed Oct 17, 2024

View reviewed changes

Guy Davenport added 5 commits October 30, 2024 19:25

fixed content mixture

2f09615

improved entities names, fixed publications

ee5ba3b

removed pedigree node

a003c0f

added to gitignore

89e45a3

fixed referencedAttribute

ca904b2

daveneti merged commit 0f0159b into LzLang-patch-1 Jan 17, 2025

		@@ -296,21 +308,14 @@
		"referencedAttribute": "availableFormats"

Conversation

daveneti commented Oct 10, 2024

Uh oh!

LzLang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daveneti commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LzLang left a comment •

edited

Loading