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

#3897 Fix error catching in blazor ApplicationContextManagers #3903

Merged

Conversation

swegele
Copy link
Contributor

@swegele swegele commented May 2, 2024

make async and match new error text after Microsoft changed it old exception text. See discussion.

Not sure about the async part if I've got that right. But I tested it and things work fine.
After upgrading to CSLA 8.1.0, before this change, I would hit the exception immediately on blazor project start (probably because of my HostedService). After this fix I am able to run the blazor project without issue. All my thousands of tests pass too.

Fixes #3897
Fixes #3890

EDIT: I also used this to get your BlazorExample working to on the list page (according to @russblair repro steps here)

async and match new error text after Microsoft changed it.
Copy link
Contributor

@StefanOssendorf StefanOssendorf left a comment

Choose a reason for hiding this comment

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

Thanks for the changes :)

@StefanOssendorf
Copy link
Contributor

Thanks for the changes. Do you happen to know if it's possible to add tests for this behavior?

@swegele
Copy link
Contributor Author

swegele commented May 2, 2024

Good question and that would be nice just in case some "helpful" person at MS decides to change the message again :-) Things are a bit fuzzy going back to 2022 but I "think" it was my HostedService in my Blazor project that was causing this to throw such that I noticed it. Obviously the BlazorExample throwing on the List page shows there are more ways to trigger it though. Anyway, my HostedService starts at project startup and runs on a timer outside of both HttpContext and a SignalR Circuit fetching a read-only list of site translation strings. After upgrading to CSLA 8.0.1 I get that error at startup (can't even get to my login/start page).

So it may be possible to mimic that somehow in a test? My gut says yes it should be do-able. It would take some fooling around on a weekend to see. Should we put in an issue for it so we don't forget?

@swegele
Copy link
Contributor Author

swegele commented May 3, 2024

@StefanOssendorf - git seems to think there is still an unresolved change request from you and I can't figure it out. Did I miss something?

@rockfordlhotka rockfordlhotka self-requested a review May 3, 2024 16:03

//check for .net 8+ error text
bool newGetCalledOutsideRazorScopeError = message.Contains(nameof(AuthenticationStateProvider.GetAuthenticationStateAsync)
+ " outside of the DI scope for a Razor component", StringComparison.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

Having hard-coded english text seems problematic - I must think that this text will vary depending on the culture of the user and/or server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered about that. Any ideas? The exception doesn't have anything else I can grab onto that I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stepping back for a moment...I guess the risk of simply testing for message.Contains(nameof(AuthenticationStateProvider.GetAuthenticationStateAsync) is low because this method is only hit on CTOR so it's not like it would be firing repeatedly over and over. What do you think about me removing everything but that on the check for the InvalidOperationException?

Copy link
Member

Choose a reason for hiding this comment

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

wow, it is amazing that they accepted hard-coded text into the repo like that.

Why do we need to be aware of the specific text? Is there some way we can shield against future changes? This seems fragile to me.

@swegele
Copy link
Contributor Author

swegele commented May 3, 2024

@rockfordlhotka does that seem a little better? That would have gotten us through the recent language change. Combine that with the fact that the type of exception is checked for as well.

@rockfordlhotka rockfordlhotka dismissed StefanOssendorf’s stale review May 3, 2024 16:42

Changes completed as requested

@rockfordlhotka rockfordlhotka merged commit cb329df into MarimerLLC:main May 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants