Graphback: Get selected fields from info in CRUD layer

[wtrocki]

Graphback generated resolvers extract selected fields from the info object in the resolver, but this is not possible to do in user-custom resolvers the getSelectedFieldsFromResolverInfo requires ModelDefinition which is not available to the user.

Solution

Pass the unedited info object to the CRUDService and extract the selected fields from here. This leads to a greatly simplified API.

return context.User.findBy(filter, context, info, page, orderBy);

[craicoverflow]

~Isn't this in the context?~

[machi1990]

@wtrocki @craicoverflow Current, to write custom resolvers that relies on the CrudService and data providers you'll need to do something like this
https://github.com/aerogear/graphback/blob/master/packages/graphback-codegen-schema/src/SchemaCRUDPlugin.ts#L474-480

we have this done for all resolvers we provide but writing custom ones can become out of hand if you have to do that all the time. What we are missing is the ability to query all fields if the context has no options.

I'll look at this tomorrow.

[wtrocki]

I'll look at this tomorrow.

Let's avoid jumping towards quick solutions. This issue is actually very big problem that might need some research first.

[machi1990]

I'll look at this tomorrow.

Let's avoid jumping towards quick solutions. This issue is actually very big problem that might need some research first.

Sure.

[wtrocki]

Spiked last evening idea of having helper method on service instead. context.User.withInfo(info).create(..)

This way we avoid problems with override etc.
Just idea

[wtrocki]

Hmm.. actually this could be tricky to do as it will require cloning service. I guess having method in context would be enough.

[craicoverflow]

Hmm.. actually this could be tricky to do as it will require cloning service.

When you say "cloning the service" do you mean returning an instance of CrudService in withInfo?

If so, what is wrong with that?

public withInfo(): CRUDService {
  this.selectedFields = ['*']

  return this;
}

Or do you mean creating a builder class?

[wtrocki]

It means returning completely new object of service.
If someone executes 2 queries that hit the same service we will override selectedFields on service itself. We hit this problem in OVP

[craicoverflow]

It is technically impossible to use getSelectedFieldsFromResolverInfo in a custom resolver because this function requires ModelDefinition as a parameter.

Even if ModelDefinition were available in resolvers, there is too much boilerplate for a user to have to write to add the selected fields to the context.

Suggestion: Let's not add selectedFields to the context, just pass the info object to the CRUDService and let the service extract this information.

There is too much manual modification of the context. We have the contextCreator specifically to not let the user control graphback context, yet here they have to do modify it.

export const noteResolvers = {
  Query: {
    findDraftNotes: async (_: any, args: any, context: GraphbackContext, info: GraphQLResolveInfo) => {
      const selectedFields = getSelectedFieldsFromResolverInfo(info, model); // <= cannot use this helper, do not have model.

      const graphback = {
        services: context.graphback.services,
        options: { selectedFields }
      };

     const results = await graphback.services.Note.findBy({ title: { contains: '[DRAFT]' } }, context, info, args.page, args.orderBy)

      return results.items;
  }
} as IResolvers

Simplified version if we pass info to the CRUDService and get it to do gather selected fields info.

export const noteResolvers = {
  Query: {
    findDraftNotes: async (_: any, args: any, context: GraphbackContext, info: GraphQLResolveInfo) => {
      const results = await context.graphback.services.Note.findBy({ title: { contains: '[DRAFT]' } }, context, info, args.page, args.orderBy)

      return results.items;
    }
  }
} as IResolvers

[machi1990]

It is technically impossible to use getSelectedFieldsFromResolverInfo in a custom resolver because this function requires ModelDefinition as a parameter.

Even if ModelDefinition were available in resolvers, there is too much boilerplate for a user to have to write to add the selected fields to the context.

Suggestion: Let's not add selectedFields to the context, just pass the info object to the CRUDService and let the service extract this information.

There is too much manual modification of the context. We have the contextCreator specifically to not let the user control graphback context, yet here they have to do modify it.

export const noteResolvers = {
  Query: {
    findDraftNotes: async (_: any, args: any, context: GraphbackContext, info: GraphQLResolveInfo) => {
      const selectedFields = getSelectedFieldsFromResolverInfo(info, model); // <= cannot use this helper, do not have model.

      const graphback = {
        services: context.graphback.services,
        options: { selectedFields }
      };

     const results = await graphback.services.Note.findBy({ title: { contains: '[DRAFT]' } }, context, info, args.page, args.orderBy)

      return results.items;
  }
} as IResolvers

Simplified version if we pass info to the CRUDService and get it to do gather selected fields info.

export const noteResolvers = {
  Query: {
    findDraftNotes: async (_: any, args: any, context: GraphbackContext, info: GraphQLResolveInfo) => {
      const results = await context.graphback.services.Note.findBy({ title: { contains: '[DRAFT]' } }, context, info, args.page, args.orderBy)

      return results.items;
    }
  }
} as IResolvers

Very nice followup. Thanks for the update.

Suggestion: Let's not add selectedFields to the context, just pass the info object to the CRUDService and let the service extract this information.

The only thing that I do not like about passing the info object into service (which should be pure business logic) is that it will feel like its invading the service layer while its something that ought to stay in the resolver (think of it as routing) layer.

Instead of passing the whole resolver info object, maybe we can just pass selected fields in a dedicated argument (that would increase the number of arguments so not that much elegant). Having said that, I am seeing the .findBy(query: GraphbackQuery) taking one argument an options GraphbackQuery object that contains:

• filter
• aggregations e.g count
• page
• orderBy
• selectedFields

With this.

1. The service layer would do its logic on this, and pass it down under to the data provider later if need.
2. Context won't be passed around different layers (not needed)
3. Context no longer modified saving us a few cpu cycles
4. GraphbackQuery object could be easily extendable and the .findBy() stays intact agains future evolutions

Slightly unrelated to what's said above and I might be going off topic. Forgive me for asking, I'd like to know what use case are we targeting with this?

[craicoverflow]

The only thing that I do not like about passing the info object into service (which should be pure business logic) is that it will feel like its invading the service layer while its something that ought to stay in the resolver (think of it as routing) layer.

Very good point there. I was thinking it would be okay to pass to the CRUD Layer to extract the selected fields, so long as the Database layer is kept clean.

Instead of passing the whole resolver info object, maybe we can just pass selected fields in a dedicated argument (that would increase the number of arguments so not that much elegant)

That would be a good middle ground. However, when https://github.com/aerogear/graphback/pull/1730/files gets merged, getSelectedFieldsFromResolverInfo will require ModelDefinition as an argument, which is not available in the the resolvers.

Instead of passing the whole resolver info object, maybe we can just pass selected fields in a dedicated argument (that would increase the number of arguments so not that much elegant).

Yep, anything outside of the context is an improvement :+1: I was looking at the context in the different layers and it is not really required for anything.

const results = await context.graphback.services.Note.findBy(filter, {
  aggregations,
  page: args.page,
  selectedFields: getSelectedFieldsFromResolverInfo(info)
})

Slightly unrelated to what's said above and I might be going off topic. Forgive me for asking, I'd like to know what use case are we targeting with this?

I was looking at https://github.com/aerogear/graphback/issues/1627 and because it is quite difficult for a user to create their own resolvers with Graphback CRUD services, I am looking for ways to improve this.

[wtrocki]

I was looking at #1627 and because it is quite difficult for a user to create their own resolvers with Graphback CRUD services, I am looking for ways to improve this.

Awesome!
My prefference to have everything in the service - this will allow us to not care but it will mean that every service call will need info object (which is actually very simple comparing to
API proposed above and requiring some separate helpers.

[craicoverflow]

Putting everything into the service presents its own challenge..lots of arguments and easy to break the API in the future. We do require some form of wrapper type to contain dynamic props but I do not believe that context is the solution.

The primary reason we have a CRUD service layer and do not use the resolvers directly is because it is extendable. I really believe the method signature should align more closely to the resolver function.

+ public findBy(args?: FindByArgs<T>, context?: GraphbackContext, info?: GraphQLResolveInfo): Promise<ResultList<T>>;
- public findBy(filter: QueryFilter<T>, context?: GraphbackContext, info?: GraphQLResolveInfo, page?: GraphbackPage, orderBy?: GraphbackOrderBy): Promise<ResultList<T>>;

interface FindByArgs<T = any>> {
  filter?: QueryFilter<T>
  page?: GraphbackPage
  orderBy?: GraphbackOrderBy
}

thoughts?

[machi1990]

Putting everything into the service presents its own challenge..lots of arguments and easy to break the API in the future. We do require some form of wrapper type to contain dynamic props but I do not believe that context is the solution.

The primary reason we have a CRUD service layer and do not use the resolvers directly is because it is extendable. I really believe the method signature should align more closely to the resolver function.

+ public findBy(args?: FindByArgs<T>, context?: GraphbackContext, info?: GraphQLResolveInfo): Promise<ResultList<T>>;
- public findBy(filter: QueryFilter<T>, context?: GraphbackContext, info?: GraphQLResolveInfo, page?: GraphbackPage, orderBy?: GraphbackOrderBy): Promise<ResultList<T>>;

interface FindByArgs<T = any>> {
  filter?: QueryFilter<T>
  page?: GraphbackPage
  orderBy?: GraphbackOrderBy
}

thoughts?

I like the idea. So +1.