Skip to content

Fix schema typeMap damaging on directiveMap generation #44

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

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

no1seman
Copy link

@no1seman no1seman commented Jan 25, 2022

Remove forcing defaultValue on directiveMap generation because it leads to adding redundant defaultValue field to Scalars itself into typeMap:

{
    "typeMap": {
        "Int": {
            "defaultValue": 1, <- ERROR
            "parseLiteral": "function: 0x415a8fe8",
            "__type": "Scalar",
            "nonNull": {},
            "serialize": "function: 0x415a8ed8",
            "isValueOfTheType": "function: 0x415a8e98",
            "name": "Int",
            "description": "..."
        }
    }
}

Here is a simple test (without this fix it fails):

function g.test_arguments_default_values_bug()
    local function callback(_, _)
        return nil
    end

    local mutation_schema = {
        ['test_mutation'] = {
            kind = types.string.nonNull,
            arguments = {
                object_arg = {
                    kind = types.inputObject({
                        name = 'test_input_object',
                        fields = {
                            nested = {
                                kind = types.string,
                                defaultValue = 'default Value',
                            },
                        },
                        kind = types.string,
                    }),
                },
                mutation_arg = {
                    kind = types.int,
                    defaultValue = 1,
                },

            },
            resolve = callback,
        }
    }

    local directives = {
        types.directive({
            schema = schema,
            name = 'timeout',
            arguments = {
                seconds = {
                    kind = types.int,
                    description = 'Request timeout (in seconds). Default: 1 second',
                    defaultValue = 1,
                },
            },
            onField = true,
        })
    }

    local root = {
        query = types.object({
            name = 'Query',
            fields = {},
        }),
        mutation = types.object({
            name = 'Mutation',
            fields = mutation_schema or {},
        }),
        directives = directives,
    }

    local compiled_schema = schema.create(root, test_schema_name)


    t.assert_equals(compiled_schema.typeMap['Int'].defaultValue, nil)
end

@DifferentialOrange
Copy link
Member

I think you should add the test into the code.

Does this fix change any directiveMap behavior?

Remove forcing defaultValue on directiveMap generation because it leads to adding redundant defaultValue field to Scalars itself into typeMap:

{
    "typeMap": {
        "Int": {
            "defaultValue": 1, <- ERROR
            "parseLiteral": "function: 0x415a8fe8",
            "__type": "Scalar",
            "nonNull": {},
            "serialize": "function: 0x415a8ed8",
            "isValueOfTheType": "function: 0x415a8e98",
            "name": "Int",
            "description": "..."
        }
    }
}
@no1seman no1seman force-pushed the fix_damaging_schema branch from 60287ca to 92e7093 Compare January 26, 2022 15:52
@no1seman
Copy link
Author

I think you should add the test into the code.

Test added

Does this fix change any directiveMap behavior?

Rather no than yes. It's a potential possibility to set defaultValue for inputObject by setting it in argument map but in this case one should use map and finally, after schema generation defaults must be injected to each argument of inputObject. I don't think that it's really will be needed for anyone. It is worth adding that it never worked in this implementation.

@Totktonada
Copy link
Member

NB: Start to collect points like 'one may expect that X works in [particular way], but that would be a misbelief'.

@no1seman
Copy link
Author

?

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

LGTM.

@Totktonada Totktonada merged commit a5476e8 into master Jan 31, 2022
@Totktonada Totktonada deleted the fix_damaging_schema branch January 31, 2022 08:05
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