Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduction in types accepted for type definitions #3592

Merged
merged 10 commits into from
Aug 17, 2023

Conversation

darrellwarde
Copy link
Contributor

@darrellwarde darrellwarde commented Jul 5, 2023

Description

Introduces a private method, normalizeTypeDefinitions, which maps to the "Type Definitions Normalization" stage of the schema generation lifecycle. It includes a commented block of what the future could look like, but for now mirrors the current functionality. We should consider how to deal with GraphQL extensions in our schema generation and translation layer, to see if we can unlock this approach.

For now, this PR just reduces the number of types we accept for type definitions, down to types which we understand and use day to day.

Complexity

Note

Please provide an estimated complexity of this PR of either Low, Medium or High

Complexity: Low

@changeset-bot
Copy link

changeset-bot bot commented Jul 5, 2023

🦋 Changeset detected

Latest commit: bedc26d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@neo4j/graphql Major
@neo4j/graphql-ogm Major
@neo4j/graphql-toolbox Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@darrellwarde darrellwarde requested a review from mjfwebb July 5, 2023 14:37
@neo4j-team-graphql
Copy link
Collaborator

neo4j-team-graphql commented Jul 5, 2023

Thanks for the Neo4j GraphQL Toolbox updates.

The Neo4j GraphQL Toolbox has now been torn down - reopening this PR will republish it.

@darrellwarde
Copy link
Contributor Author

Hmmmm, so mergeTypeDefs had a "nice" little side effect of resolving all of the object extensions and merging everything into the object. If we do take this approach, we will have to parse all of the object extensions manually - how do we feel about this? Sometimes the merging of directives resulted in weird behaviour, so maybe it's not a terrible thing. But it does mean that things are currently a little broken here.

@neo4j-team-graphql
Copy link
Collaborator

neo4j-team-graphql commented Jul 19, 2023

Performance Report

No Performance Changes

Show Full Table
name dbHits old dbHits time (ms) old time (ms) maxRows
aggregations.TopLevelAggregate 3403 3403 38 64 1134
aggregations.NestedAggregation 16553 16553 93 95 2174
aggregations.AggregationWithWhere 11979 11979 56 62 2174
aggregations.AggregationWhereWithinNestedRelationships 22116987 22116987 3327 3259 2008534
aggregations.AggregationWhereWithinNestedConnections 22116987 22116987 3025 2927 2008534
aggregations.NestedCountFromMovieToActors 9734 9734 46 66 2174
aggregations.NestedCountFromActorsToMovie 9937 9937 64 52 2174
aggregations.DeeplyNestedCount 13070335 13070331 4892 5471 2008534
batch-create.BatchCreate 4200 4200 181 195 600
batch-create.BatchCreateSmall 77 77 69 152 11
connect.createAndConnect 14424 14424 176 226 3003
connections.Connection 14082 14082 80 89 2174
connections.NestedConnection 45466 45466 122 174 4516
connections.ConnectionWithSort 3284 3284 72 89 1040
connections.ConnectionWithSortAndCypher 16325 16325 106 124 2174
create.SimpleMutation 7 7 64 88 1
cypher-directive.TopLevelMutationDirective 1135 1135 32 33 1134
delete.SimpleDelete 19401 19401 252 244 1040
delete.NestedDeleteInUpdate 19198 19198 195 214 1869
2925.SingleRelationshipFilter 6468 6468 54 54 1040
2925.NestedSingleRelationshipFilter 22900 22900 101 147 2174
2925.SingleRelationshipRequiredFilter 5201 5201 43 82 1040
2925.NestedSingleRelationshipRequiredFilter 9361 9361 67 92 1040
query.SimpleQuery 3121 3121 24 38 1040
query.SimpleQueryWithRelationship 16162 16162 49 56 2174
query.QueryWhere 9705 9684 43 69 2164
query.SimpleQueryWithNestedWhere 9883 9862 60 78 2164
query.Nested 10096041 10096041 10965 11669 2008534
query.NestedWithFilter 10074401 10074401 10975 11148 2004000
query.OrFilterOnRelationships 41746 42993 217 261 1426
query.OrFilterOnRelationshipsAndNested 35715 37321 250 313 1426
query.QueryWithNestedIn 13300 13299 64 75 1184
query.NestedConnectionWhere 9834 9834 62 93 2174
query.DeeplyNestedConnectionWhere 9949 9852 110 124 2174
query.DeeplyNestedWithRelationshipFilters 19206 19062 148 209 1620
query.NestedWithRelationshipSingleFilters 3881 3881 194 238 1134
query.Fulltext 80 80 43 52 16
query.FulltextWithNestedQuery 587 587 52 123 84
sorting.SortMultipleTypes 3515 3515 76 131 1040
sorting.SortMultipleTypesWithCypherWithCypher 14456 14507 110 230 2174
sorting.SortOnNestedFields 14082 14082 65 66 2174
sorting.SortDeeplyNestedFields 43198 43198 112 120 4516
sorting.SortWithTopLevelCypher 16162 16162 62 65 2174
unions.SimpleUnionQuery 321 321 96 77 35
unions.SimpleUnionQueryWithMissingFields 293 293 107 75 35
unions.NestedUnion 410637 410637 379 316 33033
unions.NestedUnionWithMissingFields 384611 384611 292 335 33033
update.NestedUpdate 16143 16143 104 131 2002

Old Schema Generation: 49.813s
Schema Generation: 52.278s
Old Subgraph Schema Generation: 1:02.727 (m:ss.mmm)
Subgraph Schema Generation: 1:00.860 (m:ss.mmm)

@darrellwarde darrellwarde changed the title Refined normalization of user type definitions Reduction in types accepted for type definitions Jul 19, 2023
Copy link
Contributor

@MacondoExpress MacondoExpress left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MacondoExpress
Copy link
Contributor

Maybe we have to update the documentation if we merge this!

@darrellwarde darrellwarde merged commit 2ba1d45 into neo4j:4.0.0 Aug 17, 2023
78 checks passed
@darrellwarde darrellwarde deleted the normalize-type-definitions branch August 17, 2023 09:13
darrellwarde added a commit that referenced this pull request Sep 1, 2023
* Merging content for restructuring of docs (#3087)

* test

* update

* more rearranging and merging of content

* resolving conflict

* conflict solving

* new attempt to solve conflicts

* Update dependency webpack to v5.76.1

* Update dependency react-router-dom to v6.9.0

* Update dependency lint-staged to v13.2.0

* Update dependency ws to v8.13.0

* resolving conflicts

* new attempt to solve conflicts

* new attempt to solve conflicts

* fixing content nav with pages that were missing

* further adjustments after review

* merging mathematical operators into update

* Fix yarn.lock

* undoing the accidental deletion of the neo4jgraphql.adoc page

* reviewing indexing of links after restructuring of the manual

* fix yarn

* Fix lockfile

* Fix yarn.lock

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Darrell Warde <[email protected]>

* Move subscriptions to features instead of plugins

* Tests on default subscriptions

* Update subscription docs with default example

* Add migration guide on subscriptions

* Update v4-migration.adoc

* Update class names for subscription plugins

* Update TestSubscriptionPlugin in tests

* Fix apollo e2e tests

* Apply suggestions from code review

Co-authored-by: Lidia Zuin <[email protected]>

* Rename several references from plugins to mechanisms

* Update docs/modules/ROOT/pages/subscriptions/mechanisms.adoc

Co-authored-by: Lidia Zuin <[email protected]>

* Fix escaping for relationships

* Fix cypher builder tests

* fix: use gql from graphql-tag in place of old apollo-server gql export

* Update import of gql

* rm unused import

* Remove tests against 4.3 database (#3346)

* Remove tests against 4.3 database

* Fix yarn.lock

* Fix broken Subscriptions test

* Fix property escaping in MERGE operation

* Update fluffy-suns-invite.md

* make temporary set-up to test validation rules

* field category aid

* add tests for existing graphql rule: KnownDirectiveArgument

* add support for fulltext directive in DirectiveArgumentOfCorrectType

* Remove the `@auth` directive (#3579)

* Remove `@auth` code and related tests

* Remove deprecated tests

* Remove `@auth` from final tests

* Tweaks to get tests passing

* Fix missing predicates in nested projection

* Update TCK tests following changes to authorization

* Remove commented block

* Remove redundant Subscriptions tests

* Fix unit tests

* Fix tests

* Add fulltext tests back in which were unrelated to auth

* Better Cypher Builder usage

* Fix broken tests on `4.0.0` (#3593)

* add tests and improve validation run temporary set-up

* pass extra definitions to validation

* extend DirectiveArgumentOfCorrectType to static directives - problematic

* adds rule to validate directive argument values. To be refactored

* Fix sort tck test

* Remove `Neo4jDatabaseInfo` from the public context (#3630)

* Remove Neo4j version check for point translation

* Remove Neo4j database info from public API

* Remove unneeded export, add changeset

* Fix import

* Remove req and request from our context type (#3629)

* Simpler configuration of Cypher query options (#3628)

* Perform major dependency upgrades breaking Node version (#3602)

* Perform major dependency upgrades breaking Node version

* Update OGM snapshots with new inputs and outputs

* Update cypher builder dependency to 1.0.0

* Address additional linting errors on `4.0.0` (#3654)

* Address additional linting errors on `4.0.0`

* Fix issue

* Authorization support for Apollo Federation (#3661)

* Add initial Federation authorization

* Add E2E tests

* Update Federation E2E tests to work without port

* Test type level authorization

* Add changeset

* Update TCK tests

* Skip Federation tests for GraphQL 15

* Remove auth plugin and all references (#3662)

* Remove auth plugin and all references

* Add missing dependency

* Remove auth from content nav

* Use modern driver API and bump version requirements (#3645)

* Bump driver

* In progress changes to driver usage

* Remove DriverWithSessionConfig

* Remove `getContextValuesWithBookmarks`

* Fix typo

* Fix tests

* Remove unused req from performance tests

* Remove unused variables

* Remove deprecated options

* Fix tests

* Mark bookmark fields as deprecated

* Add changeset

* Update OGM snapshots

* Un-deprecate `fulltext` argument, add descriptions for different functionality (#3669)

* Un-deprecate `fulltext` argument, add descriptions for different functionality

* Update snapshot

* Assessment and grouping of all library exports (#3670)

* Assessment and grouping of all library exports

* Add changeset

* Fix test import

* Fix doc blocks

* Simplify the programmatic toggle for debug logging (#3673)

* Simplify the programmatic toggle for debug logging

* Move Toolbox database selection

* Remove unused exports now defined in OGM (#3679)

* Remove `nodes` and `relationships` from the library's public API (#3671)

* Remove `nodes` and `relationships` from the library's public API

* Fix bad merge

* Cypher query options only settable in the context (#3674)

* Cypher query options only settable in the context

* Remove unused export

* Remove reference from troubleshooting guide

* `apoc.util.validate` to `apoc.util.validatePredicate` (#3672)

* Update math operators to use `apoc.util.validatePredicate`

* Relationship validation string using `apoc.util.validatePredicate`

* `createUpdateAndParams` array functions using `apoc.util.validatePredicate`

* `translateUpdate` to use `apoc.util.validatePredicate`, and combine the conditions for less calls

* Remove prodedure checks

* Testing to see if session is causing failures

* Add null checks to validations, only add division rule if needed

* Update packages/graphql/tests/integration/math.int.test.ts

Co-authored-by: MacondoExpress <[email protected]>

---------

Co-authored-by: MacondoExpress <[email protected]>

* Strongly type `context` field in OGM (#3690)

* Strongly type `context` field in OGM

* Fix schema tests

* Validation configuration changes (#3687)

* Miscellaneous test fix

* Missing custom resolvers to warn instead of throw

* Unified validate option, removal of library config object

* Docs updates

* Add changeset

* Update docs

* Add a bit about `startupValidation`

* Remove `neo4jValidateGraphQLDocument` (#3691)

* Subscriptions Engine (#3688)

* Changes subscription mechanism to engine

* Miscellaneous fixes

* Add migration step

* Docs remnants

* Fix linting and tests

* Update examples

* EngineAMQP to AMQPEngine

* Much better naming for subscriptions engine

* Update nine-poems-retire.md

* Re-lock

* Fix version of AMQP engine

* Version Packages (beta) (#2774)

* adds directive combination rules

* adds jwt directives validation rule

* Remove docs (#3696)

* Remove docs

* Remove references

* adds more validation rules and tests, id in progress

* finish all validations except customResolver, add tests, refactoring todo

* refactor into generic rules w/ specialized validation functions

* remove comments

* tidy up code

* improve relationship validation, start updating tests

* merge 4.0.0

* update tests

* attempt fix ts error

* fix global id inherited alias

* remove comments

* refactor relationship validation

* refactor invalid directive combinations

* remove unused imports

* update validation on getSubgraphSchema

* make valid-object-type rule work

* fix reviewdog on tests

* Version Packages (beta) (#3734)

* Schema model refactoring (#3689)

* Add directive annotations (#3615)

* feat: add alias directive annotation

* feat: add queryOptions directive annotation

* feat: parse Kind.INT and Kind.Float into JS numbers

* feat: add default directive annotation

* feat: add coalesce directive annotation

* feat: add customResolver annotation

* feat: add ID directive annotation

* refactor: use makeDirectiveNode in key-annotation tests

* feat: add mutation directive annotation

* feat: add plural directive annotation

* feat: add filterable directive annotation

* feat: add fulltext directive annotation

* feat: add node directive annotation

* feat: add populatedBy directive annotation

* feat: add query directive annotation

* feat: add private directive annotation

* feat: add relationshipProperties annotation

* feat: add selectable directive annotation

* feat: add settable directive annotation

* feat: add timestamp directive annotation

* feat: add unique directive annotation

* feat: add subscription directive annotation

* feat: add jwt-claim directive annotation

* feat: add jwt-payload directive annotation

* feat: add directives to Annotation

---------

Co-authored-by: MacondoExpress <[email protected]>

* Schema model re-design (#3596)

* initial refactor of the schema model

* add unit tests for schema model Attributes

* include initial design for the GraphQL models

* add Attribute tests, improve Attribute models

* merge schema model changes with authorization changes

* fix relationship-field parsing on schema model

* organise parser folder

* Add missing GraphQL models to the Schema Models

* change Schema Model test to showcase how to dynamically get EntityModels from relationships

* improve coverage on the Attribute Models

* add more tests to the ConcreteEntityModel

* Apply suggestions from code review

Co-authored-by: angrykoala <[email protected]>

* lint fixes

* rename graphql-models to model-adapters

* remove @relationshipProperties annotation as it is implicit within the Relationship class

* remove code duplicity around leadingUnderscores utility and test it

* parseArguments of directive using default values, add Schema Model tests for annotations and relationships

* remove not necessary node directive

* polishing

* add license header

* Update packages/graphql/src/schema-model/parser/parse-arguments.ts

* add graphqlDefaultValue to Attribute

* remove the graphqlDefault logic

* add comment on the PluralAnnotation

---------

Co-authored-by: Michael Webb <[email protected]>
Co-authored-by: angrykoala <[email protected]>

* address review

* make additional definitions mandatory on validations

* improve error check

* improve validation tests

* Update logical filters in aggregation queries

* Introspector to produce `@mutation` instead of `@exclude` (#3743)

* fix: remove unnecessary readonly (#3735)

* chore(deps): update dependency eslint-config-prettier to v8.10.0

* chore(deps): update dependency jest-extended to v4.0.1

* Introspector to produce `@mutation` instead of `@exclude`

---------

Co-authored-by: Thomas Wiss <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update logical operators on field aggregations outside of node/edge

* Update aggregation implicit and filter

* cypherParams added to new context type, and fields can be directly referred to (#3758)

* cypherParams added to new context type, and fields can be directly referred to

* Resolve all possible label references

* Address PR comments

* code review changes

* fix tests after code review changes

* Fix default logical operators in aggregation filters

* refactor: add types to assertArgumentType

* Add integration test

* fix toolbox error

* Stronger typings for context (#3779)

* Pass info into Executor rather than putting in context

* Typings for fulltext in current context

* Remove unused context entries

* Remove some unneeded context references

* Add context types

* Switch to new context typings

* Remove legacy Context type

* improve valid-directive-combination to check inherited directives as well

* remove resolutions

* fix fulltext missing type

* change id autogenerate default value, remove errors from make-augmented-schema and update tests

* add scaffold directive definition for type dependant directives

* conditionally run directiveArgumentOfCorrectType rule on auth directives

* adjust tests and invalid directive combination matrix

* fix RENAMED_DIRECTIVE_OR_TYPE regex

* remove faulty export

* Reduction in types accepted for type definitions (#3592)

* Refined normalization of user type definitions

* Remove unused type

* Wrap current behaviour with new lifecycle naming

* Fix linting error

* Delete unused tests

* Fix test typings

* Update Neo4jGraphQL.ts

* Correct default access mode for `@cypher` (#3792)

* chore(deps): update dependency eslint to v8.47.0

* Toolbox: Ensure no additional commas are added to the editor value (#3784)

* `@cypher` default access mode to match operation

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Michael Webb <[email protected]>

* Schema model improvements (#3794)

* change isListOf to includeLists flag

* clean up AttributeAdapter

* remove type guards for type

* polish

* use assertionOptions rather than flag

* fix assertions

* add isList argument to the relationship to differentiate between 1..1 or 1..n

* polish

* Move the `limit` argument of `@queryOptions` to `@limit` (#3809)

* Move the `limit` argument of `@queryOptions` to `@limit`

* Update translate-read.ts

* changes from review

* use a lambda instead of Fn.bind

* Improved typings for Subscriptions context and authorization (#3810)

* Improved typings for Subscriptions context and authorization

* Single type for common context fields

* Fix typings so it works without connectionParams

* Fix possible undefined

* Address PR comments

* Rename context interface

* add rule for directives on root type fields

* change queryOptions with limit

* chore: fix linting issues

* Remove references to global authentication (#3815)

* Version Packages (beta) (#3757)

* fix cypher with authorization on root type fields tests

* do not allow directives on interface fields except for relationshipProperties

* remove  rule as directives on intreface fields are no longer possible

* add comments

* disallow unique directive on relationshipProperties explicitly, since interface rule is gone

* improve error messages on root types fields

* remove interface inheritance from validate relationship directive rule

* remove interface inheritance from validate directive combination rule

* Breaking changes to the `@id` directive (#3823)

* Break `@id` `global` argument out into `@relayId`

* Remove arguments from `@id`

* Add changeset

* Address first PR comments

* Switch back to default name for constraints

* Fix directive description

* Remove automatic constraint for `@id`

* Fix typo

* Fix tests

* fix interface field directives tests

* fix id directive rule

* remove interface inheritance from @id validation

* move some tests to validateDocument

* fix some tests with interface inheritance

* Revert "fix some tests with interface inheritance"

This reverts commit 09069f8.

* Revert "remove interface inheritance from @id validation"

This reverts commit 29f5f85.

* Revert "fix interface field directives tests"

This reverts commit 2f0aafc.

* Revert "remove interface inheritance from validate directive combination rule"

This reverts commit 6b3175d.

* Revert "remove interface inheritance from validate relationship directive rule"

This reverts commit ae12676.

* Revert "disallow unique directive on relationshipProperties explicitly, since interface rule is gone"

This reverts commit 4c8b9e8.

* Revert "remove  rule as directives on intreface fields are no longer possible"

This reverts commit 0897e82.

* rename globalId to relayId

* refactor relationship validation to use inheritance utils

* improve directive combination to not run when < 2 directives

* allow directives on interface fields

* update tests

* Remove definitions for `@exclude`, `@readonly` and `@writeonly` (#3857)

* Change `@subscription` `operations` argument to be `events` (#3858)

* `@subscription` `operations` to `events`

* Add changeset

* Missed some usages

* Fix tests

* Rolls back TypeScript to 5.1.6 (#3867)

* update rules with object and interface extensions

* update jwt directives rule to consider extensions

* update relationshipProperties rule to consider extensions

* update relayId rule to consider extensions

* add tests on all rules for extensions

* fix skip test

* fix missing directives

* improve jwt directives rule to check for adding jwt on extension as last thing

* Fix: `cypherParams` in context now work as expected when using OGM (#3830)

* Fix: `cypherParams` in context now work as expected when using OGM

* Remove jwt and token from OGM context types

* Change test to use cypherParams

* Update typing

* improve jwt directives rule to use context document

* refactor jwt directives rule

* Fix README

* Remove unused import

---------

Co-authored-by: MacondoExpress <[email protected]>
Co-authored-by: Darrell Warde <[email protected]>
Co-authored-by: Lidia Zuin <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Darrell Warde <[email protected]>
Co-authored-by: Michael Webb <[email protected]>
Co-authored-by: a-alle <[email protected]>
Co-authored-by: Alle <[email protected]>
Co-authored-by: MacondoExpress <[email protected]>
Co-authored-by: Neo4j Team GraphQL <[email protected]>
Co-authored-by: Michael Webb <[email protected]>
Co-authored-by: Thomas Wiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants