leangen / graphql-spqr

Build a GraphQL service in seconds
Apache License 2.0
1.1k stars 182 forks source link

Maintainable way to create multiple different Inputs #199

Open pepijno opened 5 years ago

pepijno commented 5 years ago

I'm trying to figure out how to create multiple different inputs from one Java class. Suppose I have a Car class

@Data
public class Car {
    private long ID;
    private String brand;
    private String model;
}

with three mutations: addCar to add a new Car, updateCar to update a Car and deleteCar to delete a Car. Using graphql-spqr I can write something like:

@GraphQLMutation
public Car addCar(Car car) {
...
}

@GraphQLMutation
public Car updateCar(Car car) {
...
}

@GraphQLMutation
public boolean deleteCar(Car car) {
...
}

This will result more or less in the following schema:

type Mutation {
    addCar(car: CarInput) : Car
    updateCar(car: CarInput) : Car
    deleteCar(car: CarInput) : Boolean
}

type Car {
    ID: Long
    brand: String
}

type CarInput {
    ID: Long
    brand: String
}

However, the addCar mutation does not need a CarInput with an ID field, the deleteCar only requires a CarInput with an ID (it is easier to just use the ID as only input of the mutation but this is as an example). If for example the brand of the Car can only be set at initialization, the CarInput of the update should not include the brand.

I know I can create different inputs by extending the Car class and then overriding the getters and setters and adding the @GraphQLIgnore annotation, but with multiple different Inputs and more fields in the class, it quickly becomes cumbersome to create classes for each Input and overriding the getters and setters. Especially with a lot of fields in the base class. Also, changing fields in the base class takes a lot more time and is more error prone. My question is if there is an easier way to achieve multiple Inputs while keeping the java to a minimum?

kaqqao commented 5 years ago

This a cool question. I thought about it for a few hours and I think I have something decent. Will post what I have in mind as soon as I get to the computer.

kaqqao commented 5 years ago

DISCLAIMER: All this could be a terrible idea. But it might also be useful. Use judiciously.

Ok, here's a working example. It uses a custom annotation @GraphQLInputVariant to drive the mapping. You annotate an argument (of type Car in your case) to say which variant it should be, e.g. AddCarInput or DeleteCarInput, and you annotate the fields in the class to say to which variants they belong, so that AddCarInput fields only end up in AddCarInput type. You than use these two to conditionally include/exclude fields.

1) Setup


// A custom annotation used for dynamic field filtering.
// Should be split in 2. See consideration 1, at the bottom!
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE_USE, ElementType.METHOD, ElementType.FIELD})
public @interface GraphQLInputVariant {
    CarInputVariant[] value(); //Allow multiple variants
}

public enum CarInputVariant {
    AddCarInput, DeleteCarInput, UpdateCarInput
}

GraphQLSchema schema = new TestSchemaGenerator()
        .withInputFieldBuilders(new VariantAwareInputFieldBuilder(new JacksonValueMapperFactory()))
        .withTypeInfoGenerator(new DefaultTypeInfoGenerator() {
            @Override
            public String generateInputTypeName(AnnotatedType type, MessageBundle messageBundle) {
                // Variant name overrides the type name
                if (type.isAnnotationPresent(GraphQLInputVariant.class)) {
                    return type.getAnnotation(GraphQLInputVariant.class).value()[0].toString();
                }
                return super.generateInputTypeName(type, messageBundle);
            }
        })
        .withOperationsFromSingleton(new CarService())
        .generate();

    ...

// This should not be strictly needed if you're just filtering out some fields (as done here).
// A custom InclusionStrategy should be enough. But due to a stupid limitation that I'll fix in the future, this whole thing is needed.
// But, you'd still need this is you wanted more complex logic than just filtering fields (e.g. conditional non-nullness).
// But a more complex logic is probably a *very* bad idea.

public class VariantAwareInputFieldBuilder implements InputFieldBuilder {

    private final JacksonValueMapperFactory jacksonFactory;

    public VariantAwareInputFieldBuilder(JacksonValueMapperFactory jacksonFactory) {
        this.jacksonFactory = jacksonFactory;
    }

    // Delegate to an existing builder and filter some fields out conditionally.
    // You could also remap the fields in any way you want here...
    @Override
    public Set<InputField> getInputFields(InputFieldBuilderParams params) {
        return jacksonFactory.getValueMapper(Collections.emptyMap(), params.getEnvironment())
                .getInputFields(params).stream()
                // Only include the fields with no variant annotations or those that match the input variant
                .filter(field -> !params.getType().isAnnotationPresent(GraphQLInputVariant.class)
                        || !field.getTypedElement().isAnnotationPresent(GraphQLInputVariant.class)
                        || Arrays.stream(field.getTypedElement().getElement().getAnnotation(GraphQLInputVariant.class).value()).anyMatch(variant -> variant.equals(params.getType().getAnnotation(GraphQLInputVariant.class).value()[0])))
                .collect(Collectors.toSet());
    }

    @Override
    public boolean supports(AnnotatedType type) {
        return type.isAnnotationPresent(GraphQLInputVariant.class);
    }
}

2) Usage

public class CarService {

    @GraphQLMutation
    public Car addCar(@GraphQLInputVariant(AddCarInput) Car car) {
        return ...;
    }

    @GraphQLMutation
    public Car updateCar(@GraphQLInputVariant(UpdateCarInput) Car car) {
        return ...;
    }

    @GraphQLMutation
    public boolean deleteCar(@GraphQLInputVariant(DeleteCarInput) Car car) {
        return ...;
    }
}

public class Car {

    // Using public fields to keep things short
    @GraphQLQuery
    @GraphQLInputVariant({DeleteCarInput, UpdateCarInput})
    public String id;

    @GraphQLQuery
    @GraphQLInputVariant(AddCarInput)
    public String brand;

   // model field is mapped in all variants
    @GraphQLQuery
    public String model;
}

Considerations:

1) It is probably a better idea to have 2 different annotations for marking the input and for marking the fields. That would remove the currently possible but bizarre (and unhandled) case where you mark an argument with multiple variants.

2) I originally used Strings (instead of an enum) to denote the variant. But enums are better as that way you won't just misspell AddCarInput and AddCarInout and wonder why the mapping is all wrong.

3) You can also do funkier inputs, such as List<@GraphQLInputVariant(AddCarInput) Car> list with no changes

4) You could probably achieve the same by configuring Jackson filters and customizing InputFieldBuilder to use that instead of filtering manually. But I find the approach above easier...

5) I only did cursory testing on this. Seems to work as expected, but there might be some edge cases I didn't think of...

pepijno commented 5 years ago

Thanks for the quick reply, it looks like it fits exactly how my intended solution would look like and I will certainly try to see if this solution fits my application.

In your example in the getInputFields method the method getTypedElement is called on objects of type InputField, but in version 0.9.8 of spqr the class InputField does not contain a TypedElement field but it does contain the field javaType of type AnnotatedType. Is this something which will be changed in 0.9.9?

awestwell commented 5 years ago

Did you find out what branch the InputField contains the TypedElement?. I too wanted to try this solution :) There is no 0.9.9 branch/tag and 0.9.8 does not include the change. Is it possible to have this committed to a feature branch for now?

pepijno commented 5 years ago

I don't know which branch contains the TypedElement. However, using getJavaType instead of getTypedElement works just as fine.

If the fields in the classes are private and there are setters and getters then this solution doesn't fit due to how JacksonValueMapper creates the InputFields; an InputField created from a setter does not contain the annotations of the field and the annotations of the setter are not accessible. I think it is solvable using customizing the InputFieldBuilder which is what I've been trying so far.

kaqqao commented 5 years ago

The TypedElement was added in 0.9.9 (which has just been released). The reason you might need access to it is because the annotations you look for might not be applicable or present on the type itself (that you get from getJavaType) but on the underlying field/method, which is what you get from getTypedElement.

kaqqao commented 5 years ago

Do you still need this issue open or do you feel it has been sufficiently addressed?

awestwell commented 5 years ago

Good Day.

Thanks for your sample. Yes it does work based on a predefined conditions. However I am using Lombok to generate my Getters & Setters. I have placed the @GraphQLInputVariant annotation on private property definition.

The issue I am seeing is that the current code based returns a PropertyDescriptor based on the first instance found (constructor, setter, getter, field) and does not check if the annotation @GraphQLInputVariant is present before returning.

What is your suggestion in handling Lombok generating Getter and Setters and having private properties with the annotation @GraphQLInputVariant? Thanks

Ash

kaqqao commented 5 years ago

Private members are not inspected by SPQR anywhere, so it doesn't play well with Lombok. I'll likely include something in the future, but for now you're stuck with adding getters where you need them. Or writing a custom ResolverBuilder but that's non trivial.

alturkovic commented 5 years ago

Is this likely to become a new feature in the next release?

Also, specifying different input for mutations is super convenient, but the current solution you proposed requires us to write the custom annotations, generators and builders per type which is not really convenient.

Perhaps something like:

public interface Variant {
}

public @interface GraphQLInputVariant {
   Class<? extends Variant>[] value(); //Allow multiple variants
}

This would allow us to do something like:

public class CarVariants {
  public static class Add implements Variant {
  }

  public static class Update implements Variant {
  }

  public static class Delete implements Variant {
  }
}

public class MyClass {
  @GraphQLMutation
  public Car example(@GraphQLInputVariant(CarVariants.Add.class) Car car) {
    return null;
  }
}

WDYT?

kaqqao commented 5 years ago

@alturkovic

requires us to write the custom annotations, generators and builders per type

Not sure I'm getting this bit. The suggested approach (in my mind) is generic and usable on any type with no extra work. What per-type work are you referring to?

EDIT: If you meant the example using CarInputVariant, that of course is not a requirement. I'm using an enum for the OP's specific case, but you can turn that into anything you want: a string, a class or any marker at all.

As for making this into a feature... I was pondering it already... still am. But I'm not convinced this should be in mainstream usage. I see it as a workaround rather than the best way to do things. But I could be wrong... so it's not off the table.

alturkovic commented 5 years ago

I was reading about some GraphQL best practices and I like the idea of all mutations having a single input object type: https://medium.com/graphql-mastery/json-as-an-argument-for-graphql-mutations-and-queries-3cd06d252a04 https://graphqlmastery.com/blog/graphql-best-practices-for-graphql-schema-design

Not sure I'm getting this bit. The suggested approach (in my mind) is generic and usable on any type with no extra work. What per-type work are you referring to?

I was referring to your example above; the one you mentioned in your edit and explained your thinking...

As for making this into a feature... I was pondering it already... still am. But I'm not convinced this should be in mainstream usage. I see it as a workaround rather than the best way to do things. But I could be wrong... so it's not off the table.

I would argue it should be mainstream usage following the suggestions from the articles I linked above. Having some options to declare mutation input objects without having to "duplicate" our model by copy/pasting most properties would be great... Of course, as you said, this might not be the best way to do things, since both of those articles are written by the same author, but the idea clicked with me.

kaqqao commented 5 years ago

I very much like his writing too :) If you use SPQR in Relay compliant mode (generator.withRelayCompliantMutations()), all mutations will have a single input type. If the underlying method is accepting multiple arguments, a grouping type will be generated. This gives you a way to compose inputs without writing a ton of wrappers.

Give it a go, maybe that's what you're looking for :)

Hm, maybe I should enable more granular feature-toggling there...

alturkovic commented 5 years ago

I am afraid I am still not following, most likely due to lack of documentation and examples, so I am probably misusing things. This is also the first project I am trying to do with GraphQL so this is all very new to me and I might be misunderstanding basic things...

Would you mind posting a simple example of how you would reuse the model object as input (take the above Car example) without making another DTO (UpdateCar CreateCar) or listing multiple @GraphQLArgument parameters (requires copying fields from model)? Using only Car as input parameter, but requiring id field for update and ignoring id for create?

That is what I am trying to do, only one object model for all operations...

iamlothian commented 5 years ago

Some documentation or examples around input fields would be very handy. I can't find any good example that explains how input models are generated.

slavap commented 4 years ago

For similar purpose I'm using class inheritance, that way I can easily unify Query and Mutation classes. Example:

public class AuthorBase {
    @GraphQLId
    public String id;
    public String name;
    public String thumbnail;
}

public class Author extends AuthorBase {

    @GraphQLNonNull
    public String getId() {
        return id;
    }

    @GraphQLNonNull
    public String getName() {
        return name;
    }
}

@GraphQLType(name = "Author")
public class AuthorInput extends AuthorBase {
}

public class PostBase {
    @GraphQLId
    public String id;

    public String title;
    public String text;
    public String category;

    @GraphQLIgnore
    public String authorId;
}

public class Post extends PostBase {

    @GraphQLNonNull
    public String getId() {
        return id;
    }

    @GraphQLNonNull
    public String getText() {
        return text;
    }
}

@GraphQLType(name = "Post")
public class PostInput extends PostBase {
    public AuthorInput author;
}

Of course it's possible to have additional input classes for insert/update/delete (with @GraphQLIgnore adjusted fields), but it's a little bit overkill IMO :-) Generates schema like this:

type Author implements Node {
  name: String!
  thumbnail: String
  id: ID!
  posts: [Post]!
}

input AuthorInput {
  name: String
  thumbnail: String
  id: ID
}

interface Node {
  id: ID!
}

type Post implements Node {
  id: ID!
  text: String!
  title: String
  category: String
  author: Author
}

input PostInput {
  author: AuthorInput
  category: String
  title: String
  text: String
  id: ID
}