Ability to combine multiple response projections

[ruipliu]

Discussed in #984

^{Originally posted by ruipliu July 22, 2022}
This is to support complex use cases from client side building queries.

Now the response projections only have builder methods to add fields. But in client side there are certain business needs to dynamically build queries. The current client practice is creating the response projections and adding the fields in one time, which is hard coding. Could the plugin support getFields() in GraphQLResponseProjection for clients to access the fields in response projection after they created?

Here is an example:

Consider the [User - Order - Product] model.

• Use case 1: load 'user.orders.products.name' for each getUser query
• Use case 2: load 'user.orders.status' and 'user.orders.products.status' for each getUser query
• etc

In the client application, we might need data in use case 1 or use case 2 or use case 1 & 2. Hence we need to write 3 UserResponseProjection. If we have 10 use cases and their combinations exploded, we would prefer having a UserResponseProjection for each use case and combine these projections if we want to load fields in multiple use cases, which needs the fields attribute accessible.

[jxnu-liguobin]

If you don't care about to get all the values, you can use all$. Otherwise, it can't now.

[kobylynskyi]

@ruipliu if I understand correctly your problem statement, you want the ability to create a new instance of GraphQLResponseProjection out of another instance of GraphQLResponseProjection.
Is that true?
If yes, then this is a fairly simple feature to implement.

[ruipliu]

@kobylynskyi Yes correct.
Based on my knowledge of the source code, the only change is to add getFields() in GraphQLResponseProjection.java

[kobylynskyi]

Unfortunately adding getFields() method might be a breaking change for the clients who have a type with field getFields - because getFields() method will already be present in the TypeResponseProjection.

So the approach of creating an additional "copy" constructor would be better, what do you think?

Type1ResponseProjection type1 = new Type1ResponseProjection();
Type1ResponseProjection type1Copy = new Type1ResponseProjection(type1);

[ruipliu]

Sorry for the late reply.
The "copy" constructor may not work. As the aim of creating from existed one is to get the fields from a projection so that the field tree can be modified when or after creating.

For example, if we want to add price of a product in a projection having user.order.product.name field tree, we could get the product field and add accordingly.

Here are some alternative solutions, could you give some comments?

1. add special character in getFields(), like getFields$()
2. make 'fields' public

[jxnu-liguobin]

We should not modify the original getFields.

[ruipliu]

We should not modify the original getFields

There is no getFields() in current Graph l response projection. We are thinking of adding this methods to access and modify the fields.

Do you mean the field attribute should not be modified?

[jxnu-liguobin]

Do you mean the field attribute should not be modified?

we should only modify it by response projection method. If the returned by getField is mutable that will break this.

The "copy" constructor may not work. As the aim of creating from existed one is to get the fields from a projection so that the field tree can be modified when or after creating.

If the internal fields can be modified after creating object, it may lead to insecurity.

[ruipliu]

If the internal fields can be modified after creating object, it may lead to insecurity.

Could you explain the insecurity case a bit more? about what scenario and what kind of security problem. Thanks!

My feeling is the ResponseProjection is only a tool for client side building GraphQL query and it would be better to provide more convenience and accessibility. Client side could make effort to assure the security when using this class.

[jxnu-liguobin]

When a projection needs to be defined and subsequently used multiple times. Some times it may be modified, which results in multiple calls not having the same result.

[kobylynskyi]

@ruipliu,
I would really prefer not to expose fields outside. This would be breaking the encapsulation principle. So let's keep it "safe" inside the class so that users don't accidentally shoot themselves in the foot.

Let's take your case as an example:

if we want to add price of a product in a projection having user.order.product.name field tree, we could get the product field and add accordingly.

We could still solve it with the copy-constructor thing:

ParentResponseProjection proj1 = new ParentResponseProjection()
        .user(new UserResponseProjection()
                .id()
                .order(new OrderResponseProjection()
                        .total()
                        .product(new ProductResponseProjection()
                                .name()
                        )
                )
        )

ParentResponseProjection proj2 = new ParentResponseProjection(proj1) // resuing all projection from proj1
        .user(new UserResponseProjection()
                .order(new OrderResponseProjection()
                        .product(new ProductResponseProjection()
                                .price() // this will add price to an existing projection which already has name
                        )
                )
        )

ParentResponseProjection proj3 = new ParentResponseProjection(proj1) // resuing all projection from proj1
        .user(new UserResponseProjection()
                .order(new OrderResponseProjection()
                        .product(new ProductResponseProjection()
                                .none$() // this will clean up all projected fields that might already exist
                                .id()
                        )
                )
        )

// proj1 will have { user { id order { total product { name } } } }
// proj2 will have { user { id order { total product { name price } } } }
// proj3 will have { user { id order { total product { id } } } }

How does this sound to you?

[ruipliu]

Hi @kobylynskyi ,

There are two comments on your idea:

1. The current response projection class does not support deduplication of fields. If we take your idea, the fields addition mechanism would be complex, as we need to deduplicate the field tree when we add a new field.
2. I am fine with making the response projections immutable. But from your idea, if we want to build a response projection on top of another, we still need to hard code adding every fields. Shall we create a constructor or factory method that take in a list of response projection and internally combine into one? If so, we could reach an optimal position that we could maintain a response projection for each business requirement, and merge them if having multiple requirements.

Please let me know your comment. Thanks.

[kobylynskyi]

@ruipliu, thanks for your response.

To your 1st point:

The current response projection class does not support deduplication of fields. If we take your idea, the fields addition mechanism would be complex, as we need to deduplicate the field tree when we add a new field.

This should not be a problem. I think we can use a LinkedHashMap instead of a List for storing response projection fields (for deduplication purposes):

graphql-java-codegen/src/main/java/com/kobylynskyi/graphql/codegen/model/graphql/GraphQLResponseProjection.java

Line 13 in 7364f7d

protected final List<GraphQLResponseField> fields = new ArrayList<>();

To your 2nd point:

I am fine with making the response projections immutable. But from your idea, if we want to build a response projection on top of another, we still need to hard code adding every fields. Shall we create a constructor or factory method that take in a list of response projection and internally combine into one? If so, we could reach an optimal position that we could maintain a response projection for each business requirement, and merge them if having multiple requirements.

What do you mean by hard code adding every fields?
Also take in a list of response projection and internally combine into one - how does this become beneficial? Can you provide an example?
Also, note that even today you can do the following:

ProductRespProj minimalProductDetails = new ProductRespProj()
        .id().name())
ProductRespProj extendedProductDetails = new ProductRespProj()
        .id().name().price().sku())

ParentRespProj proj1WithMinimalProductDetails = new ParentRespProj()
        .user(new UserRespProj()
                .id()
                .order(new OrderRespProj()
                        .total()
                        .product(minimalProductDetails)
                )
        )
ParentRespProj proj2WithExtendedProductDetails = new ParentRespProj()
        .user(new UserRespProj()
                .id()
                .order(new OrderRespProj()
                        .total()
                        .product(extendedProductDetails)
                )
        )

And with a "constructor" thing it will make a code cleaner by extending proj2 by reusing proj1:

ParentRespProj proj2WithExtendedProductDetails = new ParentRespProj(proj1WithMinimalProductDetails)
        .user(new UserRespProj()
                .order(new OrderRespProj()
                        .product(extendedProductDetails)
                )
        )

Do you see how we can make projections more reusable? Please provide code samples if you can.

[ruipliu]

Hi @kobylynskyi , sorry for the late reply. I've consolidated the idea and let's take the below example:

Suppose we have a GQL model

account: Account
	user: Person
		name: PersonName
			first_name: string
			last_name: string
	order: Order
		product: Product
			name: string
		recipient: Person
			name: PersonName
				first_name: string
				last_name: string	

And a Java Application

The Java application is designed to listen and orchestrate data loading requirements and combine a GraphQL request. And the GraphQL service is used to load data from different data storages.

Consider the Java application supports two data loading requirements, which allows clients to select:

• A. To load names of all product purchased by this account (account.product.name)
• B. To load all person names occurred in this account (account.user.name and account.product.recipient.name)

Some of the clients needs requirement A, some needs B, while some needs A + B. They could specify their requirements through a parameter when invoking the Java application.

In the application, we need to maintain projectionA, projectionB and projectionAB like below:

AccountResponseProjection projectionA = new AccountProjection().order(
		new OrderResponseProjection().product(
				new ProductResponseProjection().name()));

PersonNameResponseProjection personNameFragment = 
		new PersonNameResponseProjection().firstName().lastName();

AccountResponseProjection projectionB = new AccountProjection()
		.user(new PersonResponseProjection().name(personNameFragment))
		.order(new OrderResponseProjection()
				.recipient(new PersonResponseProjection().name(personNameFragment)));

AccountResponseProjection projectionAB = new AccountProjection()
		.user(new PersonResponseProjection().name(personNameFragment))
		.order(new OrderResponseProjection()
				.recipient(new PersonResponseProjection().name(personNameFragment))
				.product(new ProductResponseProjection().name()));

Consider if we have tens of requirements, we should list out all combinations of requirements to make GQL call, which creates quite a lot of redundant codes.

Two possible options that could both make the response projection reusable and keep it immutable:

Option 1: Make constructor accept list of response projections

AccountResponseProjection projectionAB = new AccountProjection(Arrays.asList(projectionA, projectionB));

Option 2: Factory method to build response projections

AccountResponseProjection projectionAB = AccountResponseProjectionFactory.combine(projectionA, projectionB);

Please note that the below approach is not feasible as ResponseProjection.field is immutable.

AccountResponseProjection projectionAB = new AccountProjection(projectionB)
		.order(new OrderResponseProjection().product(new ProductResponseProjection().name());
/*
output has duplicate fields:
account {
	user { name { firstName lastName } }
	order { recipient { name { firstName lastName } } }
	order { product { name } }
}
*/

Tree iteration and field combination logic have to be implemented.

No matter which option we take, we need to add response projection combining logic.
Approaches:

1. Print out the field trees from projections
2. Iterate through trees
3. Construct a new projection

Advantage: The logic is independent to the existing response projection code, so it would not have any impact on the existing code base.

Challenge: The combination logic should be written in generic way and apply to the auto-generated projection classes.

@kobylynskyi Is my explanation clear to understand? Any comments on this approach?

[kobylynskyi]

@ruipliu I like the approach with combining multiple projections!

[ruipliu]

@kobylynskyi
May I know if you have any plan to make code change and add this feature?
otherwise I think I can contribute with a PR.

[kobylynskyi]

Yes, I am planning to work on this in the nearest days. Will keep you posted.

[kobylynskyi]

@ruipliu, plz check the initial version of the "combining" logic.
Sample unit-test that might be helpful to understand the usage and output of the joined projection:
https://github.com/kobylynskyi/graphql-java-codegen/pull/1031/files#diff-bfa4c44db3c448e71ef40d93665210a3fcbbfa17cddea25bb1e4fb3cb1f71262R73-R74

I am open to your suggestions on how to set alias / input parameters if provided values in the incoming projection objects are different. For example: { ids1: id(from: 1 to: 10) } + { ids2: id(from: 11 to: 20) }. Ideally such situations should not happen, but maybe you have any idea on which alias / input parameters we should put in the resulting projection.

Also, TBD for this feature: Kotlin and Scala support.

[ruipliu]

@kobylynskyi Thanks. I checked the code change.

From my knowledge, GraphQL service could support duplicate field with different alias. For example, this query is supported, as the alias are different:

query { 
    query: {
        ids1: id (from: 1 to: 10)
        ids2: id (from: 11 to: 20)
    }
}

The result would be query { ids1: [ xxx ], ids2: [xxx] }.

However, I'm not too sure whether field with the same alias and different parameters is supported by GraphQL service, but it is definitely a bad practice.

So my conclusion is:

• duplicate field with different alias should be kept, no matter if the parameters are the same or different
• duplicate field with same or no alias should not be acceptable, merge is needed. If parameter is different, it could not be merged and will cause problem.

Possible solution

1. make the key of LinkedHashMap<key, value> fields a pair: Pair<fieldName, alias>
2. may throw exception when field with same name and alia but different parameter is added to the fields. In other words, projection combination ability can choose not to support duplicate field with same alias and different parameters.

[kobylynskyi]

Thanks for the heads-up.
I confirm that if the field has the same (null or non-null) alias and different parameters then it leads to a problem when executing a query and the library can throw some kind of IllegalArgumentException because we know in advance that this is an invalid response projection.

[kobylynskyi]

Will include the feature in the upcoming release. Stay tuned