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

Bump client springboot version #4189

Merged

Conversation

shoothzj
Copy link
Member

@shoothzj shoothzj commented Jan 3, 2022

What's the purpose of this PR

Bump springboot version to support java17, drop the support of java7

Brief changelog

  • Mockito.any() not match null anymore, need to use Mockito.nullable() instead
  • Snake.yaml not throw parser exception on duplicate key, need to config ``
  • InvocationOnMock.getArgumentAt rename to InvocationOnMock.getArgument

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@shoothzj shoothzj force-pushed the bump-client-springboot-version branch from bd3a5de to 54291cc Compare January 3, 2022 03:55
@shoothzj shoothzj closed this Jan 3, 2022
@shoothzj shoothzj reopened this Jan 3, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2022
@shoothzj shoothzj force-pushed the bump-client-springboot-version branch from 4658097 to 7678c09 Compare January 3, 2022 04:11
@apolloconfig apolloconfig unlocked this conversation Jan 3, 2022
Comment on lines 68 to 77
StrictMapAppenderConstructor constructor = new StrictMapAppenderConstructor();
Representer representer = new Representer();
DumperOptions dumperOptions = new DumperOptions();
dumperOptions.setDefaultFlowStyle(representer.getDefaultFlowStyle());
dumperOptions.setDefaultScalarStyle(representer.getDefaultScalarStyle());
dumperOptions.setAllowReadOnlyProperties(representer.getPropertyUtils().isAllowReadOnlyProperties());
dumperOptions.setTimeZone(representer.getTimeZone());
LoaderOptions loadingConfig = new LoaderOptions();
loadingConfig.setAllowDuplicateKeys(false);
return new Yaml(constructor, representer, dumperOptions, loadingConfig);
Copy link
Member

Choose a reason for hiding this comment

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

Now that LoaderOptions provide the setAllowDuplicateKeys option, we don't need the StrictMapAppenderConstructor anymore?

Suggested change
StrictMapAppenderConstructor constructor = new StrictMapAppenderConstructor();
Representer representer = new Representer();
DumperOptions dumperOptions = new DumperOptions();
dumperOptions.setDefaultFlowStyle(representer.getDefaultFlowStyle());
dumperOptions.setDefaultScalarStyle(representer.getDefaultScalarStyle());
dumperOptions.setAllowReadOnlyProperties(representer.getPropertyUtils().isAllowReadOnlyProperties());
dumperOptions.setTimeZone(representer.getTimeZone());
LoaderOptions loadingConfig = new LoaderOptions();
loadingConfig.setAllowDuplicateKeys(false);
return new Yaml(constructor, representer, dumperOptions, loadingConfig);
LoaderOptions loadingConfig = new LoaderOptions();
loadingConfig.setAllowDuplicateKeys(false);
return new Yaml(new SafeConstructor(), new Representer(), new DumperOptions(), loadingConfig);

Copy link
Member

Choose a reason for hiding this comment

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

BTW, why would we need to set the configs of DumperOptions?

@shoothzj
Copy link
Member Author

shoothzj commented Jan 3, 2022

@nobodyiam I test the setAllowDuplicateKeys, it doesn't work. SnakeYaml library will use LoaderOptions to override the Consurtor option. If you don't pass the LoaderOptions, the default value is true. I am not sure if it's a bug or a feature.

public Yaml(BaseConstructor constructor, Representer representer) {
        this(constructor, representer, initDumperOptions(representer));
    }

    private static DumperOptions initDumperOptions(Representer representer) {
        DumperOptions dumperOptions = new DumperOptions();
        dumperOptions.setDefaultFlowStyle(representer.getDefaultFlowStyle());
        dumperOptions.setDefaultScalarStyle(representer.getDefaultScalarStyle());
        dumperOptions.setAllowReadOnlyProperties(representer.getPropertyUtils().isAllowReadOnlyProperties());
        dumperOptions.setTimeZone(representer.getTimeZone());
        return dumperOptions;
    }

    /**
     * Create Yaml instance. It is safe to create a few instances and use them
     * in different Threads.
     *
     * @param representer   Representer to emit outgoing objects
     * @param dumperOptions DumperOptions to configure outgoing objects
     */
    public Yaml(Representer representer, DumperOptions dumperOptions) {
        this(new Constructor(), representer, dumperOptions, new LoaderOptions(), new Resolver());
    }

For DumperOptions, I copy the code in YamlParser for compatibility.

Summarize, we can't config AllowDuplicateKeys through constructor param

@shoothzj
Copy link
Member Author

shoothzj commented Jan 3, 2022

@nobodyiam And If we remove the StrictMapAppenderConstructor, the testcase9 in com.ctrip.framework.apollo.util.yaml.YamlParserTest can't paas

@nobodyiam
Copy link
Member

@shoothzj

To make testcase9 work, I think we need to use SafeConstructor instead of Constructor.
BTW, I also tried the setAllowDuplicateKeys option as below and testcase2 which tests the duplicate key scenario works as expected. Is there any difference in your test case setup?

  private Yaml createYaml() {
    LoaderOptions loadingConfig = new LoaderOptions();
    loadingConfig.setAllowDuplicateKeys(false);
    return new Yaml(new SafeConstructor(), new Representer(), new DumperOptions(), loadingConfig);
  }

@shoothzj
Copy link
Member Author

shoothzj commented Jan 4, 2022

@nobodyiam I have misunderstood your meaning. I thought you want to remove the constructor, only keep the LoadOptions.
Please take a look again @nobodyiam

pom.xml Outdated Show resolved Hide resolved
@shoothzj shoothzj force-pushed the bump-client-springboot-version branch from ab0e19f to c74c3b4 Compare January 4, 2022 06:11
@codecov-commenter
Copy link

Codecov Report

Merging #4189 (c74c3b4) into master (586cc3b) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4189      +/-   ##
============================================
- Coverage     52.60%   52.54%   -0.06%     
+ Complexity     2620     2618       -2     
============================================
  Files           484      484              
  Lines         15201    15192       -9     
  Branches       1572     1571       -1     
============================================
- Hits           7996     7983      -13     
- Misses         6648     6650       +2     
- Partials        557      559       +2     
Impacted Files Coverage Δ
...m/ctrip/framework/apollo/util/yaml/YamlParser.java 94.11% <100.00%> (-0.69%) ⬇️
...ework/apollo/internals/RemoteConfigRepository.java 85.27% <0.00%> (-3.07%) ⬇️
...ervice/service/ReleaseMessageServiceWithCache.java 85.88% <0.00%> (-1.18%) ⬇️
...apollo/spring/config/PropertySourcesProcessor.java 96.36% <0.00%> (+3.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 586cc3b...c74c3b4. Read the comment docs.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Looks great!

@nobodyiam nobodyiam merged commit 02f0dca into apolloconfig:master Jan 5, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2022
@shoothzj shoothzj deleted the bump-client-springboot-version branch January 5, 2022 00:45
@nobodyiam nobodyiam added this to the 2.0.0 milestone Jan 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants