fix(#2489): Modify batchKey logic

[karatakis]

Summary:

• query > key is now used to state which query parameter is used for the batching
  If key is not present Tailcall will use the last item of batchPath
• batchPath is used to instruct Tailcall how to groupBy items

/claim #2489

Issue Reference(s):

Fixes #2489

Build & Testing:

• I ran cargo test successfully.
• I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

• I have added relevant unit & integration tests.
• I have updated the documentation accordingly. Update documentation with the updated logic for batchKey and query field tailcallhq.github.io#386
• I have performed a self-review of my code.
• PR follows the naming convention of <type>(<optional scope>): <title>

[algora-pbc]

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe.

[tusharmath]

Need to update the docs here — https://github.com/tailcallhq/tailcallhq.github.io/pulls

[codecov]

Codecov Report

Attention: Patch coverage is 74.44444% with 23 lines in your changes missing coverage. Please review.

Project coverage is 87.24%. Comparing base (9980a5a) to head (fd1d35a).
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/core/error.rs	0.00%	9 Missing ⚠️
tailcall-cloudflare/src/cache.rs	0.00%	9 Missing ⚠️
src/core/config/group_by.rs	70.00%	3 Missing ⚠️
src/core/http/request_context.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2495      +/-   ##
==========================================
- Coverage   87.29%   87.24%   -0.06%     
==========================================
  Files         253      253              
  Lines       23679    23674       -5     
==========================================
- Hits        20671    20654      -17     
- Misses       3008     3020      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[karatakis]

Next documentation changes:

• https://tailcall.run/docs/graphql-n-plus-one-problem-solved-tailcall/#using-batch-apis
• https://tailcall.run/docs/tailcall-dsl-graphql-custom-directives/#batchkey
• https://tailcall.run/docs/tailcall-dsl-graphql-custom-directives/#query-1
• https://tailcall.run/docs/graphql-grpc-tailcall/#batching

[angristan]

I'm not able to get this to work. What batching key should I use for my example in #2489? I'm trying ["bank_accounts", "id"] but it doesn't work

[bnchi]

@angristan

The PR doesn't have batchPath, the summary of this PR is misleading, the tests in the implementation are using batchKey twice and the docs doesn't seem to reflect the changes or maybe needs some enhancements ?!

https://tailcall.run/docs/tailcall-dsl-graphql-custom-directives/#batchkey-1

Here's my take on this since I spent some time trying to tackle this issue.

For your configuration to work with this PR you will need to have the following Transaction type

type Transaction {
  bank_account_id: String!
  bank_accounts: [BankAccount!]!
    @http(
      path: "/bank_accounts"
      query: [{key: "bank_account_ids[]", value: "{{.value.bank_account_id}}"}]
      batchKey: "bank_account_ids[]"
      batchKey: ["bank_accounts", "id"]
    )
  id: ID!
  slug: String!
}

Hope this helps !

[bnchi]

When batchKey is present, Tailcall considers the first query parameter to be the batch query parameter, so remember to adjust the order of the items accordingly.

Actually you might not even need to specify :

batchKey: "bank_account_ids[]"

it's a little bit of guess work tbh and the docs doesn't do a good job in describing the batch path

[karatakis]

@bnchi you are right. I need to work better on the documentation. It is true very misleading. And your configuration is correct, but you copied my mistake. Below is the fixed version.

type Transaction {
  bank_account_id: String!
  bank_accounts: [BankAccount!]!
    @http(
      path: "/bank_accounts"
      query: [{key: "bank_account_ids[]", value: "{{.value.bank_account_id}}"}]
      batchKey: ["bank_accounts", "id"]
    )
  id: ID!
  slug: String!
}

If the batchKey last value was bank_account_ids[], then there is no need for key param in the query. But because they are different you have to instruct Tailcall what you want to do.

I apologize for the inconvenience. Thanks for the feedback 🤟, and I will fix everything ASAP!

[angristan]

Thanks! My issue was that I was still deserializing to BankAccountResponse instead of [BankAccount]. So I got it working 💯

Now, I'm also noticing the same issue as #2497 (comment), there is two batched call to the API 🤔

 INFO GET http://localhost:3002/v1/bank_accounts?bank_account_ids[]=ae73e825-fea4-49d5-a25f-e2f792a8314a&bank_account_ids%5B%5D=e7f60769-8513-4215-9dde-1574b00d770b HTTP/1.1
 INFO GET http://localhost:3002/v1/bank_accounts?bank_account_ids[]=270a7f73-c941-468c-898c-6e91ac14ebc7&bank_account_ids%5B%5D=e7f60769-8513-4215-9dde-1574b00d770b HTTP/1.1

[karatakis]

The change you linked has not been merged with the main branch. If there is something that concerns you could you please open a new issue so we can track it seperate, thank you!

[angristan]

I'm talking about this PR 😄

[karatakis]

I would love to investigate it further. @angristan, can you provide me with the Tailcall configuration so I can investigate?