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

Compatible with spring boot 2.x #14729

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Compatible with spring boot 2.x #14729

merged 2 commits into from
Feb 18, 2022

Conversation

SirMin
Copy link
Contributor

@SirMin SirMin commented Jan 13, 2022

Fixes #13602.

Changes proposed in this pull request:

  1. springboot2.x config key toDashed

Copy link
Member

@TeslaCN TeslaCN left a comment

Choose a reason for hiding this comment

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

LICENSE in shardingsphere-distribution should also be updated if versions of dependencies changed.

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2022

Codecov Report

Merging #14729 (05c45b2) into master (1aacef7) will increase coverage by 0.02%.
The diff coverage is 44.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #14729      +/-   ##
============================================
+ Coverage     60.49%   60.51%   +0.02%     
  Complexity     1954     1954              
============================================
  Files          3220     3220              
  Lines         48269    48284      +15     
  Branches       8248     8254       +6     
============================================
+ Hits          29200    29219      +19     
+ Misses        16691    16673      -18     
- Partials       2378     2392      +14     
Impacted Files Coverage Δ
.../shardingsphere/spring/boot/util/PropertyUtil.java 50.00% <44.44%> (-1.12%) ⬇️
...d/text/distsql/ral/common/hint/HintSourceType.java 0.00% <0.00%> (-42.86%) ⬇️
...protocol/PostgreSQLNumericBinaryProtocolValue.java 71.42% <0.00%> (-6.35%) ⬇️
...xtended/bind/protocol/PostgreSQLByteConverter.java 51.98% <0.00%> (+7.04%) ⬆️

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 1aacef7...05c45b2. Read the comment docs.

@SirMin
Copy link
Contributor Author

SirMin commented Jan 13, 2022

LICENSE in shardingsphere-distribution should also be updated if versions of dependencies changed.

already update

Copy link
Contributor Author

@SirMin SirMin left a comment

Choose a reason for hiding this comment

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

update LICENSE

@TeslaCN
Copy link
Member

TeslaCN commented Jan 19, 2022

@ZjcNB Please help check this PR.

@zhaojinchao95
Copy link
Contributor

@ZjcNB Please help check this PR.

Thx, I will check it.

@zhaojinchao95
Copy link
Contributor

@SirMin Hello, Can you pull and merge master branch?

@SirMin
Copy link
Contributor Author

SirMin commented Jan 20, 2022

@ZjcNB

@SirMin Hello, Can you pull and merge master branch?

done

@zhaojinchao95
Copy link
Contributor

  1. upgrade snakeyaml 1.16 -> 1.29
  2. Expand the release coverage of Springframework
  3. Expand the release coverage of Spring boot
  4. Modifying unit tests

Fixes #13602.

Changes proposed in this pull request:

  • upgrade snakeyaml 1.16 -> 1.29
  • Expand the release coverage of Springframework
  • Expand the release coverage of Spring boot
  • Modifying unit tests

@SirMin Hi, thank for your submit this pull request, this idea likes good. But i hava a question.

First, Springboot 2.x not support use _ , it 's only support -, i think this Springboot problem. ShardingSphere maybe needn't to compatible it. I think throws exception is better.

@SirMin
Copy link
Contributor Author

SirMin commented Jan 22, 2022

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support.
code in pr is from spring.
image

org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm.
the springboot version is 2.3.4.RELEASE

@zhaojinchao95
Copy link
Contributor

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image

org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@SirMin
Copy link
Contributor Author

SirMin commented Jan 25, 2022

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

@zhaojinchao95
Copy link
Contributor

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

I think add toDashedForm() to resolve your problem, springboot version maybe shouldn't upgradle.

@SirMin
Copy link
Contributor Author

SirMin commented Jan 25, 2022

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

I think add toDashedForm() to resolve your problem, springboot version maybe shouldn't upgradle.

Does the unit test not cover the springboot 2.x version? if not covert I will rollback it.

@zhaojinchao95
Copy link
Contributor

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

I think add toDashedForm() to resolve your problem, springboot version maybe shouldn't upgradle.

Does the unit test not cover the springboot 2.x version? if not covert I will rollback it.

Yeah, Please rollback about springboot upgradle. only keep toDashedForm().

@SirMin SirMin force-pushed the master branch 2 times, most recently from fc67c9a to c5f7bdb Compare January 25, 2022 08:57
@SirMin
Copy link
Contributor Author

SirMin commented Jan 25, 2022

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

I think add toDashedForm() to resolve your problem, springboot version maybe shouldn't upgradle.

Does the unit test not cover the springboot 2.x version? if not covert I will rollback it.

Yeah, Please rollback about springboot upgradle. only keep toDashedForm().

done

@zhaojinchao95
Copy link
Contributor

Thanks. i will check it again.

@linghengqian
Copy link
Member

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

Does this have much to do with the snakeyaml.version ? I want to know whether the generic lost it with #15260 .

@SirMin
Copy link
Contributor Author

SirMin commented Feb 14, 2022

@ZjcNB springboot only @ConfigurationProperties not support '_', but @value support. code in pr is from spring. image
org.springframework.boot.context.properties.bind.DataObjectPropertyName#toDashedForm. the springboot version is 2.3.4.RELEASE

Yes, i see. By the way, i think add toDashedForm() for PropertyUtil.class, it's ok. But why change dependency.

@ZjcNB spring boot 2.x not support snakeyaml 1.16. need to uppgrade

Does this have much to do with the snakeyaml.version ? I want to know whether the generic lost it with #15260 .

@linghengqian yes, SET type generic lost in new snakeyaml. can use list instead of it, or set type

@zhaojinchao95
Copy link
Contributor

@SirMin This pull request has some conflicts, Can you resolve it?

@SirMin
Copy link
Contributor Author

SirMin commented Feb 16, 2022

@SirMin This pull request has some conflicts, Can you resolve it?

@ZjcNB done!

  1. Modify the private to default for unit testing
  2. add JUnit test

@menghaoranss menghaoranss added this to the 5.1.1 milestone Feb 18, 2022
@linghengqian
Copy link
Member

Does this have much to do with the snakeyaml.version ? I want to know whether the generic lost it with #15260 .

@linghengqian yes, SET type generic lost in new snakeyaml. can use list instead of it, or set type

I did not reply to you at the first time because I was busy. This is actually a bug introduced since snakeyaml 1.19 . Reference in https://bitbucket.org/snakeyaml/snakeyaml/issues/387/support-for-generic-types-when-serializing .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't compatible spring boot 2.x
6 participants