Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit batch size for entra api #34754

Merged
merged 5 commits into from
Jun 12, 2024
Merged

Conversation

jingcheng16
Copy link
Contributor

Product Description

Technical Summary

Ticket: https://dimagi.atlassian.net/browse/SAAS-15588

Microsoft Graph API batch requests are limited to 20 individual requests per batch: https://learn.microsoft.com/en-us/graph/json-batching#batch-size-limitations

So I split the list of user_ids to many chunks of size 20.

Also, the error message is not very helpful, here is the Sentry Error. So I append the raw error message from the request to the error message.

Safety Assurance

Safety story

Pretty safe. Because it didn't change much logic, it adds iteration to the batch request, and improve the error message.
Tested on production with CRS's Azure configuration

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@jingcheng16 jingcheng16 requested a review from gherceg June 11, 2024 17:28
@jingcheng16 jingcheng16 marked this pull request as ready for review June 11, 2024 17:28
@jingcheng16 jingcheng16 requested a review from biyeun as a code owner June 11, 2024 17:28
Copy link
Contributor

@gherceg gherceg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! The test failure looks related to the refactor:

corehq.tests.noseplugins.dbtransaction.UnexpectedTransaction: Was an exception raised in setUpClass() after super().setUpClass() or in tearDownClass() before super().tearDownClass()? Context: <class 'corehq.apps.sso.models.IdentityProvider'> does not have the attribute 'get_all_members_of_the_idp'

@jingcheng16
Copy link
Contributor Author

@gherceg Thank you. I fixed the test.

@jingcheng16 jingcheng16 merged commit 1941d57 into master Jun 12, 2024
13 checks passed
@jingcheng16 jingcheng16 deleted the jc/limit-batch-size-for-entra branch June 12, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants