New graphql schema #785

adriangmweb · 2021-04-13T11:35:01Z

Description

Note: This branch has been extended from #616 as the metadata reflection introduced by the TypeScript transformers is necessary. Unfortunately, this has its limitations as stated in #661.

This branch introduces the GraphQL new schema in the core, allowing filtering by more than one property at the same time and by complex properties as well in queries and subscriptions.

Changes

The GraphQL schema now is way more secure and complete, allows complex and multiple filters instead of just one basic one. Also, the filter API is way more user-friendly.

New: now it's possible to use non-compatible types like interfaces which will be transcribed as JSON types. In the future, they will be compatible with the typing

Checks

Project Builds
Project passes tests and checks
Updated documentation accordingly

Additional information

This PR solves #600 #602 and also #603. It also covers subscriptions adapters to the new filters #751

Due to a DynamoDB limitation, the schema has to make use of generic GraphQLJSONType types, which doesn't give much info to the user and doesn't give us full security when updating types. This also doesn't give us the ability yet to query by properties of a complex property that is inside a List. You can see more information about this limitation here #652

Next steps

Refactor the rest of the providers to use the new GraphQL schema, see Add new GraphQL filters to pending providers #601
Add provider agnostic integration tests for subscriptions

juanjoman

I just have a question and a request (docs) but it's looking great ✌️

docs/README.md

packages/cli/src/templates/project/tsconfig-json.ts

todo · 2021-04-15T15:14:42Z

Make retrieval of auth token cloud agnostic

booster/packages/framework-integration-tests/integration/end-to-end/read-models.integration.ts

Lines 19 to 24 in 70da128

    
             // TODO: Make retrieval of auth token cloud agnostic 
        
             await createUser(userEmail, userPassword, 'UserWithEmail') 
        
             const userAuthInformation = await getUserAuthInformation(userEmail, userPassword) 
        
             loggedClient = await graphQLClient(userAuthInformation.idToken) 
        
           })

This comment was generated by todo based on a `TODO` comment in `70da128` in #785. cc @boostercloud.

todo · 2021-04-15T15:14:45Z

make tests cloud agnostic

booster/packages/framework-integration-tests/integration/end-to-end/subscriptions.integration.ts

Lines 21 to 24 in 70da128

    
             // TODO: make tests cloud agnostic 
        
             client = await graphQLClientWithSubscriptions() 
        
           }) 
        
           after(() => {

This comment was generated by todo based on a `TODO` comment in `70da128` in #785. cc @boostercloud.

juanjoman

✌️

davidverdu

LGTM

NickSeagull

Lovely! Left some suggestions

docs/chapters/03_booster-architecture.md

NickSeagull · 2021-04-21T08:01:54Z

packages/framework-core/src/decorators/metadata.ts

+  const meta: ClassMetadata = Reflect.getMetadata('booster:typeinfo', classType)
+  if (!meta) {
+    console.log(`Could not get proper metadata information of ${classType.name}`)
+    return []


Why are we returning an empty array here? Shouldn't this fail in the case it doesn't find it?

It was failing before, and that was causing an error when creating the schema. The idea is that instead of falling, it returns an empty value, so the property type will be unknown and set as JSONObject, allowing us to have a schema when working with interfaces or custom types.

Could this be related to the inconsistencies we saw yesterday in Mayday schema?

@otoumas I guess the types (the Typescript classes) you are using in the fields messages and participants are exactly the same in both read models, right?

(BTW, Why do you have two identical read models with different names? Or was it a test?)

NickSeagull · 2021-04-21T09:03:39Z

packages/framework-core/src/services/graphql/graphql-query-generator.ts

+
+    const primitiveType = this.typeInformer.getPrimitiveExtendedType(prop.typeInfo.type)
+    if (primitiveType === Array) return this.generateArrayFilterFor(prop)
+    if (!this.generatedFiltersByTypeName[filterName]) {


Perhaps reverse this if condition and make it an early return?

NickSeagull · 2021-04-21T09:08:15Z

packages/framework-core/src/services/pub-sub/read-model-pub-sub.ts

+      case 'lt':
        if (readModelPropValue >= value) return false
        break
-      case '>':
+      case 'gt':
        if (readModelPropValue <= value) return false
        break
-      case '>=':
+      case 'gte':
        if (readModelPropValue < value) return false
        break
-      case '<=':
+      case 'lte':
        if (readModelPropValue > value) return false


Why are the conditions switched here? This looks so weird...

It's the way the subscriptions work, they definitely will need some refactoring in the future

I think (@adriangmweb correct me if I'm wrong) that right now this code is broken, as it relies on the old filters.

After a conversation with Adrián, this code is not broken

packages/framework-integration-tests/tsconfig.json

Moneyba · 2021-04-27T07:06:06Z

packages/cli/test/fixtures/mock_project/tsconfig.json

-    "emitDecoratorMetadata": true
+    "plugins": [
+      { "transform": "metadata-booster" }
+    ]


Could you explain me what does this plugin do?

@Moneyba Here is a lot of information: #616

It is a plugin we created to add more metadata to the types of the project when they are compiled from Typescript to Javascript. This way, we can generate the GraphQL schema using correctly

alvaroloes

@adriangmweb I see that the base branch is main, Should it be use-metadata-booster?

alvaroloes · 2021-04-28T11:02:19Z

packages/cli/src/templates/project/package-json.ts

@@ -37,7 +39,7 @@ export const template = `{
  "scripts": {
    "lint:check": "eslint --ext '.js,.ts' **/*.ts",
    "lint:fix": "eslint --quiet --fix --ext '.js,.ts' **/*.ts",
-    "compile": "npx tsc -b tsconfig.json",
+    "compile": "npx ttypescript -b tsconfig.json",


Shouldn't this be using the ttsc command (Ttypescript compiler) instead of ttypescript?

Yes, but when using npx with ttsc it fails on retrieving the package. That's why I'm requesting the whole package instead of using just the binary

🤔 Oh, that's when ttypescript is not installed (or it is not listed in the dependencies), so npx tries to find that package and install it. This is not ideal because it installs a "random version" (normally the latest).
I guess we should see why it is failing (it is not failing in the integration-tests package and we are using ttsc there (if I recall correctly)

Let's try to sort this, to have it ready for merge 💪🏻

After a conversation with @alvaroloes, we found out that it's necessary to use npm >= 7.0.0 in order to have npx behaving properly.

I'll update the docs

Also, turns out that npxit's not necessary when using npm scripts on the package.json file as npm is able to retrieve the binary from the root dependencies. You can see a deeper explanation here: https://www.freecodecamp.org/news/npm-vs-npx-whats-the-difference/#npm-the-package-manager

alvaroloes · 2021-04-28T11:04:33Z

packages/cli/test/fixtures/mock_project/tsconfig.json

-    "emitDecoratorMetadata": true
+    "plugins": [
+      { "transform": "metadata-booster" }
+    ]


@Moneyba Here is a lot of information: #616

It is a plugin we created to add more metadata to the types of the project when they are compiled from Typescript to Javascript. This way, we can generate the GraphQL schema using correctly

alvaroloes · 2021-04-28T11:09:05Z

packages/framework-core/src/decorators/metadata.ts

+  const meta: ClassMetadata = Reflect.getMetadata('booster:typeinfo', classType)
+  if (!meta) {
+    console.log(`Could not get proper metadata information of ${classType.name}`)
+    return []


@otoumas I guess the types (the Typescript classes) you are using in the fields messages and participants are exactly the same in both read models, right?

(BTW, Why do you have two identical read models with different names? Or was it a test?)

alvaroloes · 2021-04-28T11:11:49Z

packages/framework-core/src/services/graphql/graphql-generator.ts

@@ -151,7 +151,7 @@ function toReadModelRequestEnvelope(
    requestID: context.requestID,
    currentUser: context.user,
    typeName: readModelName,
-    filters: args,
+    filters: args.filter ? (args.filter as Record<string, ReadModelPropertyFilter>) : args,


I know you explained this to me long ago, but now I don't remember (Sorry! 😰 )

The truth is that it is strange. If args contains filter, then use that, if not we use args. Should we always use either one or the other?
If you could elaborate a bit on the reasons why this is this way I will really appreciate it.

Sure! The thing is that for being able to use filter combinators on the schema, we need to create a new GrahpQL input type where filter is the key and the properties are in the value. But, when using the filters on code-level like in commands, that is not necessary so we don't have to use the filter key.

I've tried to do it more elegant, but I didn't succeed back then.

Ok thanks for the explanation, but still don't quite get it. This code is only executed when the search is done through GraphQL. The search executed in code with Booster.readModels uses the searcher directly.

If I'm correct, this code then should be just like this:

Suggested change

filters: args.filter ? (args.filter as Record<string, ReadModelPropertyFilter>) : args,

filters: args.filter,

Let me know if I'm not seeing something 😉

Sorted 🚀

alvaroloes · 2021-04-28T11:31:07Z

packages/framework-core/src/services/graphql/graphql-query-generator.ts

+
+    const primitiveType = this.typeInformer.getPrimitiveExtendedType(prop.typeInfo.type)
+    if (primitiveType === Array) return this.generateArrayFilterFor(prop)
+    if (!this.generatedFiltersByTypeName[filterName]) {


packages/framework-core/src/services/graphql/graphql-query-generator.ts

alvaroloes · 2021-04-28T12:49:06Z

packages/framework-core/src/services/graphql/graphql-query-generator.ts

+        let nestedProperties: GraphQLInputFieldConfigMap = {}
+        const properties = getPropertiesMetadata(prop.typeInfo.type)
+        if (properties.length > 0) {
+          this.typeInformer.generateGraphQLTypeFromMetadata({ class: prop.typeInfo.type, properties })


Could you please elaborate on why is this line needed?

By calling this function the custom types like Money are created and included in the schema. Without this line, those properties would be JSONObject

Aha! Thanks. Yep, that's because Money is not included among the types of the typeInformer. I think the TypeInformer requires a little refactor to not require to pass all the types it can handle in the constructor, but to generate them as requested. But for another PR.
👏

alvaroloes · 2021-04-28T12:52:23Z

packages/framework-core/src/services/graphql/graphql-query-generator.ts

+          ...nestedProperties,
+          and: { type: new GraphQLList(this.generatedFiltersByTypeName[filterName]) },
+          or: { type: new GraphQLList(this.generatedFiltersByTypeName[filterName]) },
+          not: { type: this.generatedFiltersByTypeName[filterName] },


Woah! here my head exploded and I couldn't follow anymore. Isn't this.generatedFiltersByTypeName[filterName] undefined? We are exactly inside the if branch, which checked that that expression was undefined.

Hahaha I don't blame you! We are setting it here before the value is set. Why is that possible? It's because using fields as a function instead of as an object, the graphql library understands that it's a recursive call and the value will be set later.

There is not much info about it, but you can find an example when setting the PersonType on this example: https://graphql.org/graphql-js/type/#examples

Gotcha! Thanks for explaining

packages/framework-core/src/services/graphql/graphql-query-generator.ts

alvaroloes · 2021-04-28T13:00:49Z

packages/framework-core/src/services/graphql/graphql-query-generator.ts

+          in: { type: GraphQLList(GraphQLString) },
+          beginsWith: { type: GraphQLString },
+          contains: { type: GraphQLString },
+        }


alvaroloes

Brutal work @adriangmweb !! Merging this would be a massive improvement for the framework 👏

packages/framework-core/src/services/graphql/graphql-type-informer.ts

alvaroloes · 2021-04-28T14:56:57Z

packages/framework-core/src/services/pub-sub/read-model-pub-sub.ts

+      case 'lt':
        if (readModelPropValue >= value) return false
        break
-      case '>':
+      case 'gt':
        if (readModelPropValue <= value) return false
        break
-      case '>=':
+      case 'gte':
        if (readModelPropValue < value) return false
        break
-      case '<=':
+      case 'lte':
        if (readModelPropValue > value) return false


I think (@adriangmweb correct me if I'm wrong) that right now this code is broken, as it relies on the old filters.

alvaroloes · 2021-04-28T16:03:09Z

packages/framework-integration-tests/integration/end-to-end/subscriptions.integration.ts

+
+      const originalSubscriptionsCount = await countSubscriptionsItems()
+      // Let's create two subscriptions to the same read model
+      const observable = cartFilteredSubscription(client, { id: { eq: cartID } })


So, this is working then? Even with complex filters?

Yes, it works even better than the queries because you can filter subscriptions by ID of property inside an array! 🎉

alvaroloes · 2021-04-28T16:03:37Z

packages/framework-integration-tests/integration/end-to-end/subscriptions.integration.ts

+      const originalSubscriptionsCount = await countSubscriptionsItems()
+      // Let's create two subscriptions to the same read model
+      const observable = cartFilteredSubscription(client, {
+        cartItemsIds: { includes: productId },


Whaat? then subscriptions with the new schema are working???

Yes! 🕺🏻

...ages/framework-integration-tests/integration/providers/aws/functionality/auth.integration.ts

alvaroloes · 2021-04-28T16:05:55Z

packages/framework-integration-tests/package.json

@@ -48,7 +49,7 @@
    "watch:local": "nodemon --watch ../framework-provider-local/dist --watch ../framework-provider-local-infrastructure --watch dist --exec \"../cli/bin/run start -e local\"",
    "lint:check": "eslint --ext '.js,.ts' **/*.ts",
    "lint:fix": "eslint --quiet --fix --ext '.js,.ts' **/*.ts",
-    "compile": "tsc -b tsconfig.json",
+    "compile": "npx ttsc -b tsconfig.json",


I think this is the right command. Please, check the other pacakge.json because I think you used ttypescript instead of ttsc

Idk why but it was failing in the other one when using ttsc, that's why it's changed.

Check the other comment, I think it could be related to installing dependencies when generating the user project.

packages/framework-types/src/searcher.ts

alvaroloes

Some more missing comments

docs/chapters/03_booster-architecture.md

This reverts commit 9de34c0.

adriangmweb added feature-request New feature or request size: L Tasks that imply many files. package:core Affects the core package refactor Refactor or rearchitecture labels Apr 13, 2021

adriangmweb requested review from alvaroloes, adrian-lorenzo and a team April 13, 2021 11:35

boostercloud locked and limited conversation to collaborators Apr 13, 2021

boostercloud unlocked this conversation Apr 13, 2021

boostercloud locked and limited conversation to collaborators Apr 14, 2021

boostercloud unlocked this conversation Apr 14, 2021

juanjoman requested changes Apr 15, 2021

View reviewed changes

docs/README.md Outdated Show resolved Hide resolved

packages/cli/src/templates/project/tsconfig-json.ts Show resolved Hide resolved

adriangmweb requested a review from juanjoman April 15, 2021 15:16

boostercloud locked and limited conversation to collaborators Apr 15, 2021

boostercloud unlocked this conversation Apr 15, 2021

adriangmweb requested a review from a team April 15, 2021 15:17

juanjoman approved these changes Apr 19, 2021

View reviewed changes

davidverdu approved these changes Apr 20, 2021

View reviewed changes

NickSeagull requested changes Apr 21, 2021

View reviewed changes

adriangmweb force-pushed the new-graphql-schema branch from 70da128 to 786e45c Compare April 21, 2021 11:25

adriangmweb requested a review from NickSeagull April 21, 2021 11:26

Moneyba reviewed Apr 27, 2021

View reviewed changes

NickSeagull mentioned this pull request Apr 27, 2021

Unable to query a read model with a filter #802

Closed

adriangmweb linked an issue Apr 28, 2021 that may be closed by this pull request

Unable to query a read model with a filter #802

Closed

alvaroloes reviewed Apr 28, 2021

View reviewed changes

alvaroloes requested changes Apr 28, 2021

View reviewed changes

alvaroloes reviewed Apr 28, 2021

View reviewed changes

docs/chapters/03_booster-architecture.md Outdated Show resolved Hide resolved

docs/chapters/03_booster-architecture.md Show resolved Hide resolved

adriangmweb force-pushed the new-graphql-schema branch from 786e45c to 448203f Compare April 29, 2021 10:49

adriangmweb force-pushed the new-graphql-schema branch from 448203f to 7dd747c Compare April 29, 2021 11:04

adriangmweb requested a review from alvaroloes April 29, 2021 11:12

adriangmweb added 21 commits May 3, 2021 08:45

Revert "Revert "feat: New GraphQL schema""

99b27c1

This reverts commit 9de34c0.

refactor non compatible cases to be JSONObjects

b0824e2

handle typescript classes

35d4e07

fix operations for nested properties

efef591

update subscriptions filters

30ba05a

add properties to test the graphql filters

c0a6cba

fix unit tests

94e127f

Add graphql query filters integration tests

6701444

add filter method doc

de6006d

move subscriptions integration tests to end-to-end

d67b3fa

improve array includes method on subscriptions

e72eb1f

add subscription filters integration tests

adfaf78

fix documentation format

fb147eb

remove unnecessary tsconfig lib

39cced0

apply suggestions on documentation

cfd3f48

small refactors to improve the code

98dc3d1

improve searcher filter method doc

fcd4a4c

use ttsc and upgrade node and npm versions

6abbbe3

fix dependencies issues

90a463f

fix e2e tests

f750365

refactor graphql-generator

899f588

adriangmweb force-pushed the new-graphql-schema branch from 7dd747c to 899f588 Compare May 3, 2021 09:00

adriangmweb added 2 commits May 3, 2021 10:27

remove npx from npm scripts

24a67f1

fix graphql api unit tests

802cecb

boostercloud locked and limited conversation to collaborators May 3, 2021

boostercloud unlocked this conversation May 3, 2021

adriangmweb merged commit 67f2ebf into main May 3, 2021

adriangmweb deleted the new-graphql-schema branch May 3, 2021 09:59

	filters: args.filter ? (args.filter as Record<string, ReadModelPropertyFilter>) : args,
	filters: args.filter,

New graphql schema #785

New graphql schema #785

Conversation

adriangmweb commented Apr 13, 2021

Description

Changes

Checks

Additional information

Next steps

juanjoman left a comment

Choose a reason for hiding this comment

todo bot commented Apr 15, 2021

Make retrieval of auth token cloud agnostic

This comment was generated by todo based on a TODO comment in 70da128 in #785. cc @boostercloud.

todo bot commented Apr 15, 2021

make tests cloud agnostic

This comment was generated by todo based on a TODO comment in 70da128 in #785. cc @boostercloud.

juanjoman left a comment

Choose a reason for hiding this comment

davidverdu left a comment

Choose a reason for hiding this comment

NickSeagull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroloes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangmweb Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

alvaroloes Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroloes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroloes left a comment

Choose a reason for hiding this comment

This comment was generated by todo based on a `TODO` comment in `70da128` in #785. cc @boostercloud.

This comment was generated by todo based on a `TODO` comment in `70da128` in #785. cc @boostercloud.

adriangmweb Apr 29, 2021 •

edited

Loading

alvaroloes Apr 29, 2021 •

edited

Loading