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.

Sign up for GitHub

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

Jump to bottom

[AppConfig] Default to false if null returned when deserilizing ENABLED in feature flag setting #34091

Merged
merged 3 commits into from
Mar 20, 2023
Merged

[AppConfig] Default to false if null returned when deserilizing ENABLED in feature flag setting #34091

merged 3 commits into from
Mar 20, 2023

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Mar 17, 2023 •
edited
Loading

Description

When the ENABLED is null, use the default value false.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@mssfang mssfang self-assigned this Mar 17, 2023
@ghost ghost added the App Configuration Azure.ApplicationModel.Configuration label Mar 17, 2023
@mssfang mssfang requested a review from mrm9084 March 17, 2023 16:35
@mssfang mssfang added the Client This issue points to a problem in the data-plane of the library. label Mar 17, 2023
@mssfang mssfang added this to the 2023-04 milestone Mar 17, 2023
@mssfang mssfang marked this pull request as ready for review March 17, 2023 16:35
@mssfang mssfang requested a review from alzimmermsft as a code owner March 17, 2023 16:35
@mssfang mssfang enabled auto-merge (squash) March 17, 2023 16:36
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@mrm9084
Copy link
Member

mrm9084 commented Mar 17, 2023

The issue seems to be if a field is missing in the current implementation, you get off step, and are one ahead so nothing is equal. You should only call next if the value is checked.

The Spring integration test seems to not have a display name, so enabled was skipped, then when enabled was checked for it was already at VALUE_TRUE.

@mrm9084
Copy link
Member

mrm9084 commented Mar 17, 2023

/azp run java - spring - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mrm9084
Copy link
Member

mrm9084 commented Mar 17, 2023

This should fix the pipeline for #34022

mrm9084
mrm9084 reviewed Mar 17, 2023
View reviewed changes
...om/azure/data/appconfiguration/implementation/ConfigurationSettingDeserializationHelper.java
}
// display name
token = parser.nextToken();
if (token == JsonToken.FIELD_NAME && DISPLAY_NAME.equals(parser.getCurrentName())) {
Copy link
Member

Choose a reason for hiding this comment

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

This field is optional. The value of token here is enabled. This resulted line 202 skipping the enabled and getting the VALUE_TRUE/VALUE_FALSE, so it is never set.

@chenrujun
Copy link

Glad to see that java - spring - tests passed.

The java - appconfiguration - ci (Build Analyze) failure is not caused by current PR. Link to related PR: #33985 (comment)

@chenrujun
Copy link

/check-enforcer override

@mssfang mssfang merged commit d53f8b7 into Azure:main Mar 20, 2023
@mssfang mssfang deleted the AppConfig-bug-deserialization branch February 8, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrm9084 mrm9084 mrm9084 left review comments

@chenrujun chenrujun chenrujun approved these changes

@alzimmermsft alzimmermsft Awaiting requested review from alzimmermsft alzimmermsft is a code owner

Assignees

@mssfang mssfang

Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Milestone
2023-04
Development

Successfully merging this pull request may close these issues.
4 participants
@mssfang @azure-sdk @mrm9084 @chenrujun