-
Notifications
You must be signed in to change notification settings - Fork 45
WIP: generated relationship details for schema #899
Conversation
@wtrocki it's a rough implementation for now, but it adds the field to the generated schema, as discussed in the issue. For now it is only applying this in one direction, but it should be okay to modify this so we add details to both objects. It is also only applying these updates to the schema for now. I will add to this and will apply the changes to the generated types too. |
@@ -24,15 +21,33 @@ const getFieldParameters = (fieldName: string, type: GraphQLOutputType): any => | |||
|
|||
const getModelProperties = (model: ModelDefinition, primaryKey: string) => { | |||
const fieldMap = model.graphqlType.getFields(); | |||
const keys = Object.keys(fieldMap); | |||
|
|||
const relationships = model.relationships |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing. We do not need to use delta type and getting all the information.
r.relationForeignKey!, | ||
r.relationType | ||
); | ||
fieldOptions.relationship = r.relationType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craicoverflow can you take look if this would work from your side.
We actually getting model relationships and add extra fields to schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure yet, I think this might generate extra fields which are not part of a model. A ModelDefinition
contains relationship information for the opposite relationship field.
For example, in a 1:M relationship between Note.comments and Comment.note, the Note
model definition would have 2 relationships.
I think once we have a unit test in for this API, it will be easier to verify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're correct @craicoverflow I had to filter out the side of the relationship that I didn't need here. But the relationship info went both ways oneToMany and manyToOne
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the filtering you meant @kingsleyzissou ?
From looking at that I had thought it was filtering out the relationship information that Offix already had and did not want to apply twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's the filtering I meant @craicoverflow . As far as I can remember, we were having some issues with the comments array being included in our Datastore model and we needed to remove it. So it isn't actually duplicate data at all.
We are only adding the foreign key information the the notes so we can filter those out based on the parent id, in this case the Task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #900 - should this test be passing or am I doing something wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should be passing. I'm going to fix the linting and snapshot issues and then I can debug that
We need to fix listing and update snapshot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the side of Graphback relationship this looks good. I think having snapshots for each test case (just added along with the checking for defined bit) in createJsonSchema.test.ts
would make viewing the output much easier though.
@craicoverflow good point, I can get that added. I need to update the snapshots now anyway because I had to remove the |
92403a7
to
3aa9a26
Compare
@kingsleyzissou do you mind doing release of the cli so I can test it with some complex schemas like OVP. Going to use cli as part of the demo :) |
After testing it I see that we do not filter by @DataSync annotation. This means that we will query some id's from relationships:
but backend will not have them exposed as there is not @DataSync on the type. |
d3c2374
to
a627630
Compare
@wtrocki I've added that change we made before lunch. I will publish changes and will try verify with OVP. |
I think we need to build this into unit testing with snapshot here. |
@wtrocki coming back to this now. It looks like we did some snapshots and unit tests for this. I think verifying outside was just to see if it worked. Are we okay to merge this? |
I need:
|
* test: add test to check if manyToOne is added * fix: lint errors Co-authored-by: Gianluca Zuccarelli <gzuccare@redhat.com>
a627630
to
fcef636
Compare
I would like us to use something like OVP schema as snapshot in near future |
Description
This PR is a work in progress. The aim is to add relationship support to the generated schema as per #872
Checklist
npm test
passesnpm run build
works