Allow schema diff to use SDL document to enforce directives#3344
Allow schema diff to use SDL document to enforce directives#3344bbakerman merged 4 commits intographql-java:masterfrom ndejaco2:master
Conversation
…tion result in order to enforce directives in schema diff
|
Thanks for this PR... I am looking into it now |
| public SDLSchemaDiffSet(final String oldSchemaSdl, final String newSchemaSdl) { | ||
| this.oldSchemaSdl = oldSchemaSdl; | ||
| this.newSchemaSdl = newSchemaSdl; | ||
| } |
There was a problem hiding this comment.
why store these as Strings and then parse them in the getter. At this point you cant know they are valid documents
I think it would be better if it was private SDLSchemaDiffSet(final Document oldSchemaSdl, final Document newSchemaSdl)
Also make this constructor private. We have static constructor methods and the change in signature I mentioned above would be possible with private constructors and a breaking change when the they are public. private constructors on classes should be a starting point
There was a problem hiding this comment.
Sounds good to me. I do wonder if its worth it to have two classes: SDLSchemaDiffSet and IntrospectionSchemaDiffSet when they both will just be created with documents. I think in this case it would be better to merge it as a single class and just add a flag in the builder that introspection will not properly enforce directives? I went ahead and did it that way, let me know what you think.
| public IntrospectionSchemaDiffSet(Map<String, Object> introspectionOld, Map<String, Object> introspectionNew) { | ||
| this.introspectionOld = introspectionOld; | ||
| this.introspectionNew = introspectionNew; | ||
| } |
There was a problem hiding this comment.
again as discussed below - store this as a Document not a Map<String, Object> (I know the old code did but we get a chance to be better with new code)
and make this constructor private
src/main/java/graphql/schema/diff/IntrospectionSchemaDiffSet.java
Outdated
Show resolved
Hide resolved
2 participants
This PR is made to fix a bug (#3343) with SchemaDiff where the enforceDirectives option is not working properly.