Kaqqao parsed directive arguments

[bbakerman]

This is related to #1229 - I could not work out how to push to @kaqqao original branch - so I made my own

The original issue is #1228

This takes his ideas and uses GraphqlDirective as the runtime object for directives and arguments rather than Map<String,Map<String,Object> representation.

So now given

{
    books(author: "John Doe") @cache(enabled = true) {
        isbn
        title
    }
}

a data fetcher can do this

DataFetcher df =  env -> {
   GraphqlArgument arg = env.getDirectiveArgument("cache","enabled)
   boolean enabled = arg == null ? false : Boolean.valueOf(arg.getValue())
     
}

[bbakerman]

This was stopping the command line version from building ebacuse of JavaDoc errors. Clearly our builds dont run the same profile of JavaDoc as I do. Not sure why

[andimarek]

Looks good overall. See my comments.

Also: I think a small end to end test with getting the directives from the DFE would be useful.

[andimarek]

LinkedHasMap

[andimarek]

LinkedHashMap

[kaqqao]

Gave this a bit more thought, and I think there should be a method that returns a map of parsed directives for any directive container. This is because one might need to get directives from a different field than the first (if DataFetchingEnvironment#getFields contains multiple fields) or from a fragment or fragment definition.

I can do it if you agree on the approach.

[bbakerman]

@andimarek - there is an integration test inside graphql.execution.DirectivesResolverTest

[bbakerman]

Gave this a bit more thought, and I think there should be a method that returns a map of parsed directives for any directive container. This is because one might need to get directives from a different field than the first (if DataFetchingEnvironment#getFields contains multiple fields) or from a fragment or fragment definition.

I can do it if you agree on the approach.

@kaqqao - what shape do you see for this? as in what is the key? The Field node? Should be not just combine them??

Do you need the AST nodes per set of directives?

{
    books(author: "John Doe") @cache(enabled = true) {
        isbn
        title
    }
   also : ...FragX
}
 
fragment FragX {
   books @burnAfterReading {
     title
   }
}

[kaqqao]

@bbakerman The way I have it right now is that I return a new object (called Directives in my case) that is a glorified Map<Introspection.DirectiveLocation, Map<String, List<GraphQLDirective>>.

I.e. it is a map of directives by name by location. The reason there's a List is because the same directive (same name) could appear multiple times but with potentially different argument values. So the directives for both books fields in your example are merged inside the inner Map<String, List<GraphQLDirective>> (under DirectiveLocation.FIELD), they're not keyed by the AST node.

Similarly to how only the current fields' directives are available, so are only the current fragments' directives (where a current fragment is a fragment to which a current field belongs).

E.g. for a complex query such as:

fragment Details on Book @timeout(afterMillis: 25) {
    title
    review @timeout(afterMillis: 5)" +
}

query Books @timeout(afterMillis: 30) {
    books(searchString: "monkey") {
        id
        ...Details @timeout(afterMillis: 20)
        ...on Book @timeout(afterMillis: 15) {
            review @timeout(afterMillis: 10)
        }
    }
}

The resulting map for the review field would be:

{
  "FIELD": {
    "timeout": [ {"afterMillis": 5}, {"afterMillis": 10} ]
  },
  "MUTATION": {},
  "INLINE_FRAGMENT": {
    "timeout": [ {"afterMillis": 15} ]
  },
  "FRAGMENT_DEFINITION": {
    "timeout": [ {"afterMillis": 25} ]
  },
  "FRAGMENT_SPREAD": {
    "timeout": [ {"afterMillis": 20} ]
  },
  "QUERY": {
    "timeout": [ {"afterMillis": 30} ]
  }
}

For the id field, there would be nothing under INLINE_FRAGMENT , FRAGMENT_DEFINITION or FRAGMENT_SPREAD as that field belongs to no fragments. Also nothing under FIELD as it doesn't have its own directives either. It would only have the QUERY directive.

Not sure if that's the way it should be, that's just the way I have it now...

I then have some convenience methods on this new Directives class for getting a single directive or the whole list per name per location.

I don't really have a formed opinion yet on what should be in the library and how it should look... but I'm guessing in the background a structure keyed by the AST node will be necessary, while in the client-visible part it could be for the current context only, similar to the thing laid out above.

This is of course if such a feature is desired in the first place, as it's rather complicated 😟

[kaqqao]

I added a comprehensive example and more rationale to the previous message, so if you're reading this from an email, please make sure to see the updated version of it.

[bbakerman]

@kaqqao - yes I think you are right. We do need this and not what I have here.

If you are happy to then can you please make a PR, unwind the directives support we have there (directives of the top level field) and put in your idea.

The name graphql.Directives is already taken.

My suggestion would be something like EncounterDirectives say.

I would then see the DataFetchingEnvironemnt have a EncounterDirectives getEncounteredDirectrives method that is your access to the dfirectives on the ASDt nodes for that field.

Wew would also need to capture these into ExecutionStepIngo so one can look up as the parent directives.

[bbakerman]

please keep the DirectivesResolver code but change it to cater for the new collection of directives.

Also please add tests for this new scenario

[bbakerman]

I might back this change out since its 1/2 complete as is - not invaluable but not what we want as API know that I think more on it.

[bbakerman]

This has now been reverted via #1329

@kaqqao - if you want to have a go at implenting it again (starting with the original code as a base) then please do so.