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

Introduce ConfigurationValidationException #2076

Merged
merged 8 commits into from
Jun 7, 2023

Conversation

ciaozhang
Copy link
Contributor

ConfigurationManager won't cache configuration if config is invalid and ConfigurationValidationException

@sruke
Copy link
Contributor

sruke commented May 11, 2023

@ciaozhang, consider creating a GitHub issue for this PR with a summary of the issue and why this change is needed. This will help with updating the release notes with relevant information.

Copy link
Contributor

@dannybtsai dannybtsai left a comment

Choose a reason for hiding this comment

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

Don't use hard-coded "null", use Microsoft.IdentityModel.Tokens.Utility.Null instead.

Copy link
Contributor

@dannybtsai dannybtsai left a comment

Choose a reason for hiding this comment

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

Why renaming this class? The original ConfigurationValidationException is thrown when a configuration fails to validate, which seems right to me.

Copy link
Contributor

@victorm-hernandez victorm-hernandez left a comment

Choose a reason for hiding this comment

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

🕐

{
theoryData.ExpectedException.ProcessException(ex, context);
// this should throw, because last configuration retrieved was null
Assert.Throws<AggregateException>(() => configuration = configurationManager.GetConfigurationAsync().Result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert

Let's add a check here, if the theoryData.PresetCurrentConfiguration is set to true, throw since this should never be the case.

Copy link
Contributor

@victorm-hernandez victorm-hernandez left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@brentschmaltz brentschmaltz left a comment

Choose a reason for hiding this comment

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

We are throwing a new exception that does not derive from a previously thrown exception, we need to derive InvalidConfigurationException from the exception that was previously thrown.

Copy link
Contributor

@TimHannMSFT TimHannMSFT left a comment

Choose a reason for hiding this comment

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

Please either unify jitter or cut a PBI to unify jitter before checking in. We shouldn't proliferate different ways of doing this in our code without a plan to address.

Approving with the expectation you'll open an issue before merging. (same goes for pending error message marking as PII but that's probably easier to just fix).

@ciaozhang
Copy link
Contributor Author

@ciaozhang ciaozhang merged commit 48aab10 into dev Jun 7, 2023
1 of 2 checks passed
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.

None yet

6 participants