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

Catch exception precisely, and change an api url #3278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rayluo
Copy link

@rayluo rayluo commented Mar 3, 2025

{PR title}

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars):

Before this change, the ViewData["json"] was not populated.

@rayluo rayluo requested a review from a team as a code owner March 3, 2025 12:09
@rayluo rayluo force-pushed the rayluo/refactor branch from 7878704 to f3c7300 Compare March 3, 2025 12:10
Copy link

github-actions bot commented Mar 3, 2025

Summary

Summary
Generated on: 3/3/2025 - 12:21:49 PM
Coverage date: 3/3/2025 - 12:20:59 PM - 3/3/2025 - 12:21:44 PM
Parser: MultiReport (3x Cobertura)
Assemblies: 0
Classes: 0
Files: 0
Line coverage:
Covered lines: 0
Uncovered lines: 0
Coverable lines: 0
Total lines: 0
Covered branches: 0
Total branches: 0
Method coverage: Feature is only available for sponsors

Coverage

No assemblies have been covered.

Copy link

github-actions bot commented Mar 3, 2025

Summary

Summary
Generated on: 3/3/2025 - 12:24:29 PM
Coverage date: 3/3/2025 - 12:23:34 PM - 3/3/2025 - 12:24:23 PM
Parser: MultiReport (3x Cobertura)
Assemblies: 0
Classes: 0
Files: 0
Line coverage:
Covered lines: 0
Uncovered lines: 0
Coverable lines: 0
Total lines: 0
Covered branches: 0
Total branches: 0
Method coverage: Feature is only available for sponsors

Coverage

No assemblies have been covered.

);
ViewData["name"] = user.DisplayName;
}
catch (Exception)
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually no, you should not do that. as there is an attribute to catch the exception.
Did you see the [AuthorizeForScopes(Scopes = new[] { "user.read" })] attribute, @rayluo ?

See Managing incremental consent and conditional access for details

Copy link
Author

@rayluo rayluo Mar 4, 2025

Choose a reason for hiding this comment

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

It is true that the sample still works as-is without this part of change.

The reason I made this change is because I ran into an exception in await _graphServiceClient.Me.GetAsync(...) after I experimentally changed the "BaseUrl" in appsettings.json from https://graph.microsoft.com/beta to https://graph.microsoft.com/v1.0/me. Presumably the _graphServiceClient.Me.GetAsync(...) is only expected to work with that preconfigured MS Graph beta endpoint. In any case, this change shall make the sample more robust.

Copy link
Author

@rayluo rayluo Mar 4, 2025

Choose a reason for hiding this comment

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

@jmprieur , if we take the context above into consideration, would you suggest wrapping this code path with exception handling, or not? Please let me know and I can make the changes.

UPDATE: Please refer to repro step 2 in #3284

Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

blocking due to JM's comment

@jmprieur
Copy link
Collaborator

jmprieur commented Mar 4, 2025

@rayluo : let's start to raise a GitHub issue with the customer problem (and repro steps)?
Then we can decide. For the moment I don't understand what the problem is

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.

DevApps WebAppCallsMicrosoftGraph improvement suggestions
3 participants