Skip to content

implement a new checkForTypeExtensionFieldUniqueness#4034

Closed
xuorig wants to merge 1 commit intographql-java:masterfrom
xuorig:field-extension-uniqueness-perf
Closed

implement a new checkForTypeExtensionFieldUniqueness#4034
xuorig wants to merge 1 commit intographql-java:masterfrom
xuorig:field-extension-uniqueness-perf

Conversation

@xuorig
Copy link

@xuorig xuorig commented Jul 2, 2025

I'm afraid this might not be backward compatible / a suitable user experience. But curious on your thoughts. This check is very, very expensive on large schemas (containing a lot of extensions such as federated schemas).

This proposed solution is not quite the same as before. The current check compares. each extension against each other extension. This means that if two types define a, we get two errors for a.

Could we get away with calling out redefinitions only once?

@bbakerman
Copy link
Member

bbakerman commented Aug 2, 2025

I had a look into this and your change makes sense.

Today we might catch the following

        type Query {
            foo : Foo
        }
        
        type Foo {
            foo : String
        }
        
        extend type Foo {
           redefinedField : String
        }
        
        extend type Foo {
           otherField1 : String
        }
        
        extend type Foo {
           otherField2 : String
        }
        
        extend type Foo {
           redefinedField : String
        }
        
        extend type Foo {
           redefinedField : String
        }

0 = {TypeExtensionFieldRedefinitionError@3141} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"
1 = {TypeExtensionFieldRedefinitionError@3142} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"
2 = {TypeExtensionFieldRedefinitionError@3143} "'Foo' extension type [@11:9] tried to redefine field 'redefinedField' [@12:12]"
3 = {TypeExtensionFieldRedefinitionError@3144} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"
4 = {TypeExtensionFieldRedefinitionError@3145} "'Foo' extension type [@11:9] tried to redefine field 'redefinedField' [@12:12]"
5 = {TypeExtensionFieldRedefinitionError@3146} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"

That results in 6 errors - because for each extend type Foo - it checks that each one doesnt extend with respect to each other one

So this is indeed a N * M error checking and for large schemas - this is going to be slow

@bbakerman
Copy link
Member

bbakerman commented Aug 2, 2025

Putting the above test onto this branch results not in less errors but in more

0 = {TypeExtensionFieldRedefinitionError@3147} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"
1 = {TypeExtensionFieldRedefinitionError@3148} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"
2 = {TypeExtensionFieldRedefinitionError@3149} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"
3 = {TypeExtensionFieldRedefinitionError@3150} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"
4 = {TypeExtensionFieldRedefinitionError@3151} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"
5 = {TypeExtensionFieldRedefinitionError@3152} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"
6 = {TypeExtensionFieldRedefinitionError@3153} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"
7 = {TypeExtensionFieldRedefinitionError@3154} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"
8 = {TypeExtensionFieldRedefinitionError@3155} "'Foo' extension type [@23:9] tried to redefine field 'redefinedField' [@24:12]"
9 = {TypeExtensionFieldRedefinitionError@3156} "'Foo' extension type [@27:9] tried to redefine field 'redefinedField' [@28:12]"

Hmmm I need to look at the "order complexity" of both sets of code.

@bbakerman bbakerman mentioned this pull request Aug 4, 2025
Merged
@bbakerman
Copy link
Member

bbakerman commented Aug 4, 2025

Closed because #4076 tsakes this code and improves it slightly

@bbakerman bbakerman closed this Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xuorig @bbakerman