Skip to content

Skip complexity calculation by directiveEstimator when astNode is undefined #21

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

hnq90
Copy link
Contributor

@hnq90 hnq90 commented Oct 11, 2019

Currently, I'm using Apollo Server 2 with Federation.

In QueryComplexity.ts, I found that field in estimatorArgs could have undefined value in astNode property.

And if we use DirectiveEstimator, args.field.astNode in this line could be undefined and make getDirectiveValues throws error.

So it's better to set default value of field.astNode to an empty object {}.

These are which I printed from field:

_entities:
{ description: '',
    type: [_Entity]!,
    args: [ [Object] ],
    resolve: [Function],
    subscribe: undefined,
    deprecationReason: undefined,
    extensions: undefined,
    astNode: undefined,
    name: '_entities',
    isDeprecated: false,
    [Symbol()]: true },
_service:
{ description: undefined,
    type: _Service!,
    args: [],
    resolve: [Function],
    subscribe: undefined,
    deprecationReason: undefined,
    extensions: undefined,
    astNode: undefined,
    name: '_service',
    isDeprecated: false,
    [Symbol()]: true },

Update: Skip complexity calculation by directiveEstimator when astNode is undefined

@hnq90 hnq90 force-pushed the bug/fix_ast_node_undefined branch 2 times, most recently from 0e7a064 to 8e2d1cd Compare October 11, 2019 01:20
@hnq90 hnq90 force-pushed the bug/fix_ast_node_undefined branch from 8e2d1cd to 963f5d3 Compare October 14, 2019 00:56
Copy link
Collaborator

@ivome ivome left a comment

Choose a reason for hiding this comment

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

Would you mind adding a simple test case to prevent regression in the future?

@hnq90 hnq90 force-pushed the bug/fix_ast_node_undefined branch from 963f5d3 to 9eb9d11 Compare October 16, 2019 03:54
@hnq90 hnq90 changed the title Prevent astNode's value is undefined when perform _entities & _service query Skip complexity calculation by directiveEstimator when astNode is undefined Oct 16, 2019
@hnq90 hnq90 force-pushed the bug/fix_ast_node_undefined branch from 9eb9d11 to 81dd9b9 Compare October 16, 2019 04:36
@ivome ivome merged commit 23c1ed8 into slicknode:master Oct 21, 2019
@hnq90 hnq90 deleted the bug/fix_ast_node_undefined branch October 21, 2019 02:14
@ivome
Copy link
Collaborator

ivome commented Oct 21, 2019

This is published with v0.4.1. Thanks for the PR @hnq90

@hnq90
Copy link
Contributor Author

hnq90 commented Oct 21, 2019

@ivome Thank you. 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants