Skip to content

Conversation

dekkku
Copy link
Contributor

@dekkku dekkku commented Jul 22, 2024

Summary:
problem: when we specify the batchKey: ["a","b"] we essentially expect the path to target and assume the last value to be the query param key.
eg.
in the discussion example, batchKey: ["bank_accounts", "id"] , tells the server id will be used for the extraction of json item from batched endpoint's response and id will be used form a batched end point which isn't the case when extraction property name is different than the name used to form batched endpoint. so we need more information in order to solve this problem.

proposed solution:

      batch: {batchKey: "bank_account_ids[]", extractionPath: ["bank_accounts", "id"]}

here we tell form the endpoint with batchKey and use extraction path for extract the json object from batched endpoint.
i.e when creating batched endpoint use bank_account_ids[] and in the batched endpoint response go to bank_accounts.id to figure out merging.

Issue Reference(s):
Fixes #2489
/claim #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.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: fix Iterations on existing features or infrastructure. label Jul 22, 2024
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.30%. Comparing base (9980a5a) to head (2c8a0a0).
Report is 1 commits behind head on main.

Files Patch % Lines
src/core/config/group_by.rs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2497   +/-   ##
=======================================
  Coverage   87.29%   87.30%           
=======================================
  Files         253      253           
  Lines       23679    23678    -1     
=======================================
  Hits        20671    20671           
+ Misses       3008     3007    -1     

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

@angristan
Copy link

This is really, really nice! 🫶

I was able to check this PR locally and test it out. It's even better than I expected, because you basically solved my two concerns from #2471.

Although it fits my use case, I feel like making the batching work with nested resources should be separeted from "extracting" or flattening the resource. For example, it's the not case here but I could want to access meta (or any other value) that is alongside bank_accounts in the response while still having the batching work.

More importantly it seems there's an batching issue 🤔

INFO POST http://localhost:3002/v1/transactions HTTP/1.1
 INFO GET http://localhost:3002/v1/membershipsfilters%5Bids%5D%5B%5D=2696aeb5-eb94-45e5-8bb1-3cd163a1b668&filters%5Bids%5D%5B%5D=5182052c-180e-451a-ab48-1874f83d03d7&filters%5Bids%5D%5B%5D=66f1bbf4-89f0-4c5c-becd-2fc5d1fb6947&filters%5Bids%5D%5B%5D=f601e869-4384-4df6-a286-39e95ce6ce08 HTTP/1.1
 INFO GET http://localhost:3006/v1/teams/?ids[]=4e3a67d5-1856-4fb3-9aa6-d4238d6e9c01&ids%5B%5D=5df40b69-a4d0-448e-85ec-3429a7815c34 HTTP/1.1
 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

Here I am also batching other resources (here memberships and teams), which seem to work fine. But there is two calls to bank_accounts with two IDs each time, one of which is the same in the two calls.

@dekkku
Copy link
Contributor Author

dekkku commented Jul 23, 2024

@angristan yes, i agree with you that problem exists. when you do batching it basically strips out all the contents from API response and only keeps the json object mentioned in extraction_path.
regarding duplicate api calls, i'm not sure why its happening i have added assertions with test.

@tusharmath
Copy link
Contributor

Thanks @dekkku closing it in favour of #2495

It was a much smaller and simple change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🙋 Bounty claim type: fix Iterations on existing features or infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Batching with nested HTTP responses

3 participants