Skip to content

Fix infinite loop on error in user group fetch request#21597

Open
laurenastrid1 wants to merge 1 commit intomainfrom
laurenastrid1/userGroupInfiniteLoop
Open

Fix infinite loop on error in user group fetch request#21597
laurenastrid1 wants to merge 1 commit intomainfrom
laurenastrid1/userGroupInfiniteLoop

Conversation

@laurenastrid1
Copy link
Contributor

Description

Code Changes Checklist

  • New or updated unit tests added
  • All existing tests pass (npm run test)
  • Code follows contributing guidelines
  • Telemetry/logging updated if relevant
  • No regressions or UX breakage

Reviewers: Please read our reviewer guidelines

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves reliability of Azure AD group fetching by ensuring the Microsoft Graph pagination loop exits when a request fails, preventing a potential infinite loop during group enumeration.

Changes:

  • Exit the while (nextUrl) pagination loop when makeGetRequest throws, rather than continuing with an unchanged nextUrl.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

PR Changes

Category Target Branch PR Branch Difference
vscode-mssql VSIX 6414 KB 6414 KB ⚪ 0 KB ( 0% )
sql-database-projects VSIX 7061 KB 7061 KB ⚪ 0 KB ( 0% )
data-workspace VSIX 535 KB 535 KB ⚪ 0 KB ( 0% )

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.85%. Comparing base (fed2757) to head (549a667).

Files with missing lines Patch % Lines
extensions/mssql/src/azure/utils.ts 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #21597      +/-   ##
==========================================
- Coverage   72.85%   72.85%   -0.01%     
==========================================
  Files         331      331              
  Lines       99070    99071       +1     
  Branches     5546     5546              
==========================================
  Hits        72175    72175              
- Misses      26895    26896       +1     
Files with missing lines Coverage Δ
extensions/mssql/src/azure/utils.ts 51.80% <0.00%> (-0.32%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nextUrl = response.data["@odata.nextLink"];
} catch (error) {
console.error("Error fetching user groups:", error);
break; // Exit the loop on error
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with the response here; but presumably response.data["@odata.nextLink"] won't work in the next iteration?

Or in other words, if response.data["@odata.nextLink"] may work to get groups, should we set nextUrl in the catch block and try again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things that we need to handle here:

  1. if getting the response throws
  2. if parsing the response throws

Probably need multiple try-catch blocks in here

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.

6 participants