Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

WIP: generated relationship details for schema #899

Merged
merged 24 commits into from
Nov 23, 2020
Merged

Conversation

kingsleyzissou
Copy link
Contributor

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 passes
  • npm run build works
  • tests are included
  • documentation is changed or added

@kingsleyzissou
Copy link
Contributor Author

@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
Copy link
Contributor

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;
Copy link
Contributor

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

Copy link

@craicoverflow craicoverflow Oct 13, 2020

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.

cc @kingsleyzissou

Copy link
Contributor Author

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

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 ?

https://github.com/aerogear/offix/blob/schema-relationships/packages/datastore/cli/src/generate-documents/createJsonSchema.ts#L27

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.

Copy link
Contributor Author

@kingsleyzissou kingsleyzissou Oct 13, 2020

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.

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?

Copy link
Contributor Author

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

@wtrocki
Copy link
Contributor

wtrocki commented Oct 13, 2020

We need to fix listing and update snapshot

Copy link

@craicoverflow craicoverflow left a 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.

@kingsleyzissou
Copy link
Contributor Author

@craicoverflow good point, I can get that added. I need to update the snapshots now anyway because I had to remove the _deleted field (it was causing a 400 error).

@wtrocki
Copy link
Contributor

wtrocki commented Oct 19, 2020

@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 :)

@wtrocki
Copy link
Contributor

wtrocki commented Oct 19, 2020

After testing it I see that we do not filter by @DataSync annotation. This means that we will query some id's from relationships:

      "distributionCentreId": {
        "type": "string",
        "key": "distributionCentreId",
        "relationship": "DistributionCentre"
      },
      "volunteerId": {
        "type": "string",
        "key": "volunteerId",
        "relationship": "Volunteer"
      },

but backend will not have them exposed as there is not @DataSync on the type.

@kingsleyzissou
Copy link
Contributor Author

@wtrocki I've added that change we made before lunch. I will publish changes and will try verify with OVP.

@wtrocki
Copy link
Contributor

wtrocki commented Oct 22, 2020

I think we need to build this into unit testing with snapshot here.
Testing outside will not work in long term. Any issue we seen and case we handle should be in this repo

@kingsleyzissou
Copy link
Contributor Author

@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?

@wtrocki
Copy link
Contributor

wtrocki commented Nov 23, 2020

I need:

  1. Rebase.
  2. npm publish --tag=dev
  3. Then I will verify the changes in OVP and we can merge

@wtrocki
Copy link
Contributor

wtrocki commented Nov 23, 2020

I would like us to use something like OVP schema as snapshot in near future

@wtrocki wtrocki merged commit f84688b into master Nov 23, 2020
@wtrocki wtrocki deleted the schema-relationships branch November 23, 2020 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants