Skip to content

Conversation

@igdianov
Copy link
Collaborator

@igdianov igdianov commented Aug 17, 2019

This PR adds support for per request schema transformation and instrumentation the uses GraphQLExecutorContextFactory interface:

public interface GraphQLExecutorContextFactory {

    GraphQLExecutorContext newExecutorContext(GraphQLSchema graphQLSchema);

}

in order to create instance of GraphQLExecutorContext interface implemenation that will apply customizations to GraphQL schema field visibility, execution context, and instrumentation before executing query via:

public interface GraphQLExecutorContext {
    
    ExecutionInput.Builder newExecutionInput();

    GraphQL.Builder newGraphQL();

    GraphQLSchema getGraphQLSchema();
    
}

And then in QraphQLExecutor.execute(..)

    /* (non-Javadoc)
     * @see org.activiti.services.query.qraphql.jpa.QraphQLExecutor#execute(java.lang.String, java.util.Map)
     */
    @Override
    @Transactional(TxType.REQUIRED)
    public ExecutionResult execute(String query, Map<String, Object> arguments) {
        Map<String, Object> variables = Optional.ofNullable(arguments)
                                                .orElseGet(Collections::emptyMap);
        
        GraphQLExecutorContext executorContext = contextFactory.newExecutorContext(graphQLSchema);

        ExecutionInput.Builder executionInput = executorContext.newExecutionInput()
                                                               .query(query)
                                                               .variables(variables);        

        GraphQL.Builder graphQL = executorContext.newGraphQL();
        
        return graphQL.build()
                      .execute(executionInput);
    }

There is Spring Boot auto-configuration support for GraphQLContext, GraphqlFieldVisibility and Instrumentation via Java's Supplier functional interface, to make it possible use @RequestScope magic to inject Supplier functions per request, session, application, i.e.

@Bean
@RequestScope
public Supplier<GraphQLContext> graphqlContext(HttpServletRequest request, @AuthenticationPrincipal UserDetails user) {
    return () -> GraphQLContext.newContext()
                               .of("request", request)
                               .of("user", user)
                               .build();
}

TODOs

  • Add test coverage
  • Add documentation in PR with example

Fixes #160
Fixes #89
Fixes #197

@igdianov igdianov self-assigned this Aug 17, 2019
@codecov
Copy link

codecov bot commented Aug 17, 2019

Codecov Report

Merging #163 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #163     +/-   ##
===========================================
+ Coverage     68.11%   68.32%   +0.2%     
- Complexity      498      505      +7     
===========================================
  Files            33       35      +2     
  Lines          2578     2595     +17     
  Branches        431      431             
===========================================
+ Hits           1756     1773     +17     
  Misses          644      644             
  Partials        178      178
Impacted Files Coverage Δ Complexity Δ
...a/query/schema/impl/GraphQLJpaExecutorContext.java 100% <100%> (ø) 4 <4> (?)
.../schema/impl/GraphQLJpaExecutorContextFactory.java 100% <100%> (ø) 1 <1> (?)
...phql/jpa/query/schema/impl/GraphQLJpaExecutor.java 100% <100%> (ø) 5 <4> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bf7596...a03c051. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 17, 2019

Codecov Report

Merging #163 into master will decrease coverage by 0.01%.
The diff coverage is 73.8%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #163      +/-   ##
============================================
- Coverage     70.67%   70.66%   -0.02%     
- Complexity      934      942       +8     
============================================
  Files            62       65       +3     
  Lines          4351     4421      +70     
  Branches        661      661              
============================================
+ Hits           3075     3124      +49     
- Misses          998     1019      +21     
  Partials        278      278
Impacted Files Coverage Δ Complexity Δ
...oconfigure/GraphQLControllerAutoConfiguration.java 100% <100%> (ø) 1 <0> (ø) ⬇️
...utoconfigure/GraphQLJpaQueryAutoConfiguration.java 100% <100%> (+12.5%) 1 <0> (-2) ⬇️
...jpa/query/schema/GraphQLExecutionInputFactory.java 100% <100%> (ø) 1 <1> (?)
...phql/jpa/query/schema/impl/GraphQLJpaExecutor.java 100% <100%> (ø) 4 <2> (+1) ⬆️
...entures/graphql/jpa/query/example/Application.java 18.18% <12.5%> (-31.82%) 2 <1> (ø)
.../schema/impl/GraphQLJpaExecutorContextFactory.java 52% <52%> (ø) 4 <4> (?)
...a/query/schema/impl/GraphQLJpaExecutorContext.java 88.46% <88.46%> (ø) 4 <4> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36703f0...6c1d9a2. Read the comment docs.

@igdianov
Copy link
Collaborator Author

@molexx This is the PR to provide per request schema transformation to be able to apply field visibility function based on user profile in the execution context injection for query security. I will try implement security example using schema directives. Let me know what you think.

@igdianov igdianov changed the title feat: add initial GraphQLJpaExecutorContextFactory implementation feat: add support for per request schema transformation and instrumentation Aug 18, 2019
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch 2 times, most recently from a601390 to ff0559c Compare August 24, 2019 14:04
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch from ff0559c to e089299 Compare August 31, 2019 22:08
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch 2 times, most recently from 129a9e7 to 9fb44de Compare September 11, 2019 04:14
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch 2 times, most recently from c0a7287 to 7146699 Compare October 6, 2019 22:19
@molexx
Copy link
Contributor

molexx commented Oct 15, 2019

Thanks @igdianov , sorry I haven't had time to play with this until this week.

Are your comments in the first post still valid after your later changes?
I've built with this branch, I'm calling schemaBuilder.fieldVisibility() with my own GraphqlFieldVisibility subclass but the methods in it are not called when loading the schema in graphiql.

graphql-java doesn't mention GraphqlFieldVisibility until v12 - although the class is in v11 maybe it isn't used until v12?

If you think it should work then maybe my fieldVisibility is not being kept as part of the schema merge in GraphQLSchemaFactoryBean? If you think that might be the case I'll investigate, otherwise I might have to wait for the update to graphql-java 13.

@igdianov
Copy link
Collaborator Author

@molexx Thanks for feedback. I have not tried this yet and I don't know if graphqlFieldVisibility applies to graphq-java 11 or not. I will build a test case to see what is going on...

@igdianov igdianov force-pushed the igdianov-executor-context-factory branch from 7146699 to d963dd6 Compare October 21, 2019 00:19
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch 2 times, most recently from 4b007f1 to 1329528 Compare December 1, 2019 09:15
@molexx molexx mentioned this pull request Dec 2, 2019
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch 3 times, most recently from 642b84f to f2ca2ea Compare December 16, 2019 07:01
@igdianov igdianov force-pushed the igdianov-executor-context-factory branch from f2ca2ea to de717a7 Compare December 29, 2019 19:37
@molexx
Copy link
Contributor

molexx commented Dec 31, 2019

What do you think about this branch @igdianov? I like it because it provides a way to set the GraphQLFieldVisibility which I need to get working at some point, and we need it to set runtime context/environment properties that can be used by RowFilters.

Also it fixes #75.

@igdianov
Copy link
Collaborator Author

igdianov commented Jan 2, 2020

@molexx I had a break to work on this today. I have refactored the implementation to apply Spring Boot auto-configuration support for GraphQLContext, GraphqlFieldVisibility and Instrumentation via Java's Supplier, so it will be possible use @RequestScope magic to inject Supplier functions per request, session, application, i.e.

@Bean
@RequestScope
public Supplier<GraphQLContext> graphqlContext(HttpServletRequest request, @AuthenticationPrincipal UserDetails user) {
    return () -> GraphQLContext.newContext()
                               .of("request", request)
                               .of("user", user)
                               .build();
}

I will need to add more tests and then merge it to master. Feel free to check it out and add your test cases.

@molexx
Copy link
Contributor

molexx commented Jan 2, 2020

Ah looks great, thanks! I'll try it out.

@duiscairt-foobar
Copy link

This PR adds support for per request schema transformation and instrumentation the uses GraphQLExecutorContextFactory interface:

public interface GraphQLExecutorContextFactory {

    GraphQLExecutorContext newExecutorContext(GraphQLSchema graphQLSchema);

}

in order to create instance of GraphQLExecutorContext interface implemenation that will apply customizations to GraphQL schema field visibility, execution context, and instrumentation before executing query via:

public interface GraphQLExecutorContext {
    
    ExecutionInput.Builder newExecutionInput();

    GraphQL.Builder newGraphQL();

    GraphQLSchema getGraphQLSchema();
    
}

And then in QraphQLExecutor.execute(..)

    /* (non-Javadoc)
     * @see org.activiti.services.query.qraphql.jpa.QraphQLExecutor#execute(java.lang.String, java.util.Map)
     */
    @Override
    @Transactional(TxType.REQUIRED)
    public ExecutionResult execute(String query, Map<String, Object> arguments) {
        Map<String, Object> variables = Optional.ofNullable(arguments)
                                                .orElseGet(Collections::emptyMap);
        
        GraphQLExecutorContext executorContext = contextFactory.newExecutorContext(graphQLSchema);

        ExecutionInput.Builder executionInput = executorContext.newExecutionInput()
                                                               .query(query)
                                                               .variables(variables);        

        GraphQL.Builder graphQL = executorContext.newGraphQL();
        
        return graphQL.build()
                      .execute(executionInput);
    }

There is Spring Boot auto-configuration support for GraphQLContext, GraphqlFieldVisibility and Instrumentation via Java's Supplier functional interface, to make it possible use @RequestScope magic to inject Supplier functions per request, session, application, i.e.

@Bean
@RequestScope
public Supplier<GraphQLContext> graphqlContext(HttpServletRequest request, @AuthenticationPrincipal UserDetails user) {
    return () -> GraphQLContext.newContext()
                               .of("request", request)
                               .of("user", user)
                               .build();
}

TODOs

  • Add test coverage
  • Add documentation in PR with example

Fixes #160
Fixes #89
Fixes #197

Hi, any plans to merge this? We use this on a branch and it works well. It would be great if we could go back to master!
Thanks

@igdianov igdianov force-pushed the igdianov-executor-context-factory branch from 541a41f to 6c1d9a2 Compare January 21, 2020 07:53
@igdianov igdianov merged commit ae541cb into master Jan 21, 2020
@igdianov igdianov deleted the igdianov-executor-context-factory branch January 21, 2020 08:00
@igdianov
Copy link
Collaborator Author

@duiscairt-foobar @molexx It is merged. Feel free to open a PR if need to fix something. :)

@molexx
Copy link
Contributor

molexx commented Jan 21, 2020

Ah great thanks!

@duiscairt-foobar
Copy link

Thanks 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: Metrics Instrumentation Hook Restricting data based on user Implement Directive instrumentation support

3 participants