Adding the ability to call input object setter methods if they exist, falling back to direct field access.

[ehardy]

As per the discussion in #1088

Pull request checklist

• Please read our contributor guide
• Consider creating a discussion on the discussion forum
  first
• Make sure the PR doesn't introduce backward compatibility issues
• Make sure to have sufficient test cases

Pull Request type

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Other (please describe):

Changes in this PR

Adding the ability in DefaultInputObjectMapper to call setter methods if they exist on input objects, falling back to the current behaviour of setting fields directly.

Alternatives considered

None

[berngp]

@ehardy thanks for the PR! Our CI is getting stuck, not sure why, so the builds are failing. Will try to address this soon.

I haven't had time to deeply walk through the changes, but a few things come to mind...

1. Is there something from Spring's or Spring Boot that we could leverage (e.g. Data Binders )
2. Wondering if we can reduce the number of objects that get allocated when we are attempting to resolve the setters of the given type. Setters are resolvable via the class and don't need to know each unique object reference, i.e. as long as two objects share the same type they share the same setters. You are using the PropertyAccessorFactory which will allocate a new BeanWrapper per new bean, even if we are processing N elements of the same bean.

[ehardy]

Thanks @berngp for the feedback! Here are my thoughts about your questions:

1. Had a look at DataBinder. Under the hood, it uses the same ConfigurablePropertyAccessor (of which BeanWrapper is an implementation) mechanism that I have used to either call setter methods or do direct field access. So I don't think it provides much compared to the current solution, unless you'd like to review the input object binding process more in depth, in which case it could possibly simplify some things.
2. As I was implementing the PR, I thought that object allocation might be a concern. Out of curiosity, is this is a general concern within DGS? Also, I suppose you're thinking about the situation where a collection of input objects need to be bound? The solution I implemented is actually a bit worse than what you described in terms of object allocation ;) There's one instance of the Accessor class, plus one ConfigurablePropertyAccessor for setter access, plus a second ConfigurablePropertyAccessor for field access, so 3 objects total, for each object that needs to be data bound. First of all, do you think the Accessor class I created is a good abstraction over setter/field access? If the answer is no, then it is a different discussion! But if the abstraction makes sense, then I think we could do without the Spring ConfigurablePropertyAccessor abstractions and use the Java reflection facilities directly. That would get rid of the two ConfigurablePropertyAccessor objects within Accessor, with the tradeoff of more code/handcrafted logic within it. The net result would be a single instance of Accessor per input object that needs to be bound, instead of 3. If you'd like to reduce allocation even further, possibly those Accessor objects could be cached per Class object type within DefaultInputObjectMapper.

Let me know what you think!

[berngp]

Thanks @ehardy for the reply, and again thanks for the PR.

I suppose you're thinking about the situation where a collection of input objects need to be bound?
Yes, I am thinking of a large number of elements in a collection that are of the same type. I wouldn't want us to mainly understand if we are caching the method setters for the given type and we just reuse them for the field.

It might be worth also creating a benchmark like this.

[ehardy]

I like the idea of a benchmark, let's get actual numbers, will look into that. Also, I have started working on an alternate version of Accessor that would not rely on Spring's ConfigurablePropertyAccessor but would do reflection directly on the input object's Class object. Let's see how that goes, hopefully will submit updates to the PR soon.

[ehardy]

Hello @berngp , just a quick update on progress. I have managed to rework the implementation of Accessor from my previous PR to use raw Java reflection, for the most part. There is still one failing unit test in InputArgumentTest that has to do with handling generic collection parameters containing generic collections of a parameterized type. It seems there is more parameter information available at the field level than at the method parameter level for some reason. Spring and ConfigurablePropertyAccessor is able to figure it out somehow, so there has to be a way. Will keep looking.

[berngp]

Hi @ehardy , thanks for all the effort in the PR. Could you please push a branch when you have the chance? I'm a bit worried you are having to re-implement a lot due my previous suggestion. Would like to get an idea on the scope of the work.

[ehardy]

Hey @berngp , I found how to deal with the situation I described above. It seems that in class hierarchies, not all Method instances are equal. Method instances in base classes have more type information than their equivalent in subclasses, especially for generic parameter types. So all tests are now passing. I haven't pushed the changes to this PR yet, you can see them here. I believe you should have access, let me know if that is not the case. Let me know what you think of this version.

As I was reviewing before pushing this last set of changes, I thought of a small variation that might be more elegant. If you notice in this version of Accessor, there are a few repetitive method/field lookups, as well as if/else for dealing with Method objects vs Field, which I'm not too fond of. I'm thinking the lookup could be done once by Accessor, and it could return instances of a Property class, of which there would be two subclasses: MethodProperty and FieldProperty. The logic for type determination and property setting, which varies slightly depending on what type of class member you're dealing with, would then be implemented in its respective subclass, eliminating if/elses.

As usual, let me know what you think of all of that! I haven't gotten to implement the benchmark as you suggested. Will look into it.

[ehardy]

@berngp Here's this 3rd alternate version that I alluded to in my previous comment.

[berngp]

@ehardy, haven't been able to review but will do early next week. Happy to add the benchmarks so we can have a better understanding of degradation, if any.

[ehardy]

Hello @berngp , have you had the time to review the different alternatives? Let me know which one, if any, would be the preferred one and if any changes are required.

[berngp]

@ehardy very sorry for the delay, will look into this in the following days so we can merge it soon.

[ehardy]

Hello @berngp , did you get a chance to look into this? Anything I could help further with?

[geordiegeoff]

Hi all, we are using the framework (which is great by the way) and we were really looking forward to progress on this as we work with mutations in a similar manner. Essentially, similar to a PATCH, data fields are ignored if they are not in the request but the presence of a value or an explicit null either updates the value or removes it from the DB. Obviously accessing mutations via your library is not functioning correctly as things currently stand so I guess my questions are...

a) Has this been, or is due to be, released?
b) If it has, which version is it applicable from and how is it configured (or expected to be configured) to behave as outlined above?

Thanks in advance.

[srinivasankavitha]

@geordiegeoff - I'm not sure I understand the issue you are having with mutations. That said, could you please try with the latest 7.x version of the DGS framework? We did make changes over the past year wrt how input field mapping works, so maybe give it a try?

If you still see issues, could you please open a new issue with the description of problem you are experiencing and steps to reproduce, that would be very helpful.

[ehardy]

Hello @srinivasankavitha, the problem, described in #1088, is that DGS does not distinguish between input fields that were explicitly set to null from fields that were absent from the GraphQL request. Oftentimes there is some business logic that needs to distinguish between the two, such as should the backing DB fields be set to null or ignored and be left as is. The idea behind the PR I had opened, making it so that DGS could call setter methods when present, was to allow the use of a wrapping type at the field level that could detect if the field was set or not. You can find an example in the discussion.

We are using DGS 7.x and as far as I can tell the issue is still present. Would be happy to refresh the PR if all of this still makes sense, let me know.

[geordiegeoff]

So the issue is exactly as @ehardy eloquently describes. We may be able to schedule in a version update but it appears that the problem still stands so would not be a solution. Essentially we have had to implement a fairly 'hacky' workaround in which we...

• exclude the offending input schema fragments from code generation
• create our own classes representing these schema fragments which all extend from Map.
• If the setter is called the field name and value are placed in the map. This value can be explicitly set to null to indicate that value should be removed from the DB.
• If the setter is never called then that field is never placed into the Map and therefore is omitted from the serialization of the GraphQL request entirely, which means that field is not included in any update to the DB. Effectively it is 'skipped' entirely.

Not the most elegant solution but it works, mainly thanks to your support for Map serialization...

[srinivasankavitha]

Thanks @ehardy and @geordiegeoff. That makes sense to me, and appreciate your patience with this discussion. @ehardy - I like your solution of using a strategy to determine this behavior so we don't break compatibility with existing behavior. If you are able to refresh the PR for the latest code, I'd be happy to review and test. We won't be able to prioritize this fix for this quarter, but any help would be greatly appreciated!

[ehardy]

Sure thing! Also need to find some time, but will get to it 😁 Thank you @srinivasankavitha

[ehardy]

@srinivasankavitha I have refreshed the PR from latest master branch, please have a look at your convenience. As I had mentioned, I'm not too well versed in Kotlin, so my implementation might feel too Java-ish 😁 Something that had been mentioned in my previous exchanges on the PR was to implement a JMH benchmark. Do you think it would be useful/necessary? Let me know.

[srinivasankavitha]

Thanks for the updates, will review this week.

[srinivasankavitha]

@kilink - would appreciate your input on this PR as well when you get a chance.

[ehardy]

Hello, I refreshed the branch from last master. Is there anything else needed to move this forward? Let me know.

[srinivasankavitha]

Hi @ehardy - Thanks for the changes. We've had to put this on hold for a bit due to upcoming refactoring that @kilink is planning to work on which may address some of these issues as well. I'll let @kilink comment further on that.

[ehardy]

Any updates that could be shared?

[kilink]

Hey @ehardy, thank you for the PR and apologies for the slow movement on this. There is a larger refactor of the InputObjectMapper that I hope to land soon that is intended to fix long-standing pain points and simplify things going forward. I also think your proposed change could be much smaller and more localized if we lean on existing utilities / helper that exist in Spring (see PropertyAccessorFactory). I think it might be easier to circle back once we land the refactor, if you're still interested in pursuing this.

[ehardy]

Sounds good @kilink. About your comment about using Spring's PropertyAccessorFactory and the like, the funny thing is that my initial PR used just that. See the very first comment, there were some concerns about object allocation. In terms of scope of change, it was much smaller indeed. Unfortunately this version of the PR got lost along the way, but it would be easy to resurrect.

I'll keep monitoring. Please keep this thread updated once the refactor lands.

[kilink]

@ehardy FYI the aforementioned refactoring has landed see #1735

[ehardy]

Very nice @kilink ! Will look updating this PR shortly.

[ehardy]

@kilink I have updated the implementation following your recent changes in DefaultInputObjectMapper. I have moved back to leveraging Spring's PropertyAccessor. I believe this is the simplest implementation. Please have a look.

[ehardy]

Thanks for the feedback @kilink! I have addressed comments and updated the PR. Would it be possible to have another look?

[kilink]

LGTM! Thanks for bearing with me there, I know this process took a while. I appreciate the contribution!

[ehardy]

I have rebased the PR. What else is required in order to get it merged?

[kilink]

I asked @srinivasankavitha to take a look as well.

Looks like the build is failing due to a lint error, you should be able to fix it just by running ./gradlew formatKotlin

[srinivasankavitha]

I asked @srinivasankavitha to take a look as well.

Looks like the build is failing due to a lint error, you should be able to fix it just by running ./gradlew formatKotlin

The changes look great to me. Thanks for all the work and your patience through the process. As @kilink mentioned, there is a lint error, but we can merge as soon as we have a green build

[ehardy]

Lint error has been fixed, hopefully the build passes now. Build and tests pass locally. Thanks for the feedback throughout.