Partition calls

[felipe-gdr]

Please make sure you consider the following:

• [ yes ] Add tests that use __typename in queries
• [ yes ] Does this change work with all nadel transformations (rename, type rename, hydration, etc)? Add tests for this.
• [ FF can be in AGG] Is it worth using hints for this change in order to be able to enable a percentage rollout?
• [ yes ] Do we need to add integration tests for this change in the graphql gateway?
• [ maybe ] Do we need a pollinator check for this?

[sbarker2]

Nice work 👍

[sbarker2]

Love the comprehensive test coverage

[temaEmelyan]

I think atlassian public schema defines Payload as

"""
The general shape of a mutation response.
"""
interface Payload {
    "Was this mutation successful"
    success: Boolean!

    "A list of errors if the mutation was not successful"
    errors: [MutationError!]
}

would it be reasonable to check if the type extends Payload

[felipe-gdr]

I considered it, but eventually decided not to do it because the type for the use case we're targeting doesn't actually implement Payload. But you make a good point, that type should really be implementing this interface. They're already adhering to the contract, so it shouldn't be a big change for them 👍

[gnawf]

Left some comments, we can address them later too.

In general looks good.

[gnawf]

Would like to see this made into an interface so it's easier to parse e.g.

interface NadelPartitionKeyExtractor {
  fun getPartitionKey(
        scalarValue: ScalarValue<*>,
        inputValueDef: GraphQLInputValueDefinition,
        context: Any,
    ): String
}

[felipe-gdr]

Nice.

[gnawf]

Should be internal so code outside library can't access it.

[gnawf]

Don't think errorClassification is ever variable, so we should hard code it into the super call i.e.

class NadelPartitionGraphQLErrorException(
    message: String,
    path: List<Any>? = null,
) : NadelGraphQLErrorException(message, path, partitioningErrorClassification)

[gnawf]

Should be internal

[gnawf]

Are the generics here correct?

Should just be emptyList<Any?>() right?

[gnawf]

Multiple parent nodes should only be if it's a list in the result. Don't think abstract types would cause that.

[felipe-gdr]

if it's a list in the result

Hm, when would that happen? I'm deailng with fields that have type of List and parentNodes is always 1 🤔

[gnawf]

Could be

&& this.fields.all { it.name == "success" || it.type.unwrapNonNull() is GraphQLList }

Since .filter and other transform functions return an entire new List unless the transform is on a Sequence

[gnawf]

Could be

.flatMap {
  if (it == null) {
    emptyList()
  } else {
    check(it is List<*>) { "Expected a list, but got ${it::class.simpleName}" }
    it
  }
}

Also I think that ClassCastException will print out the expected/actual type. So could just straight up cast it.

.flatMap {
  it as List<*>?
}

[gnawf]

Nice

[gnawf]

Maybe be explicit about it being the engineSchema since there's a few schema objects.

[felipe-gdr]

Left some comments, we can address them later too.

In general looks good.

Thanks for the great suggestions 🙏

I'll merge it as is to unblock subsequent changes quickly and raise follow-up PRs to address your feedback.