Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintainable way to create multiple different Inputs #199

Open
pepijno opened this issue Nov 20, 2018 · 14 comments

Comments

Projects
None yet
4 participants
@pepijno
Copy link

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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);
    }
}
  1. 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

This comment has been minimized.

Copy link
Author

commented Nov 22, 2018

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

This comment has been minimized.

Copy link

commented Dec 11, 2018

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

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

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

@awestwell

This comment has been minimized.

Copy link

commented Dec 17, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

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

This comment has been minimized.

Copy link

commented Jan 20, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

@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

This comment has been minimized.

Copy link

commented Jan 20, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

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

This comment has been minimized.

Copy link

commented Jan 20, 2019

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.