fix(spring): update setters for properties of type Duration #1093
Conversation
…rse argument of different type
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
| getterAndSetter.add( | ||
| createDurationSetterMethod(thisClassType, propertyName, propertyType)); | ||
| } else { | ||
| getterAndSetter.add(createSetterMethod(thisClassType, propertyName, propertyType)); |
There was a problem hiding this comment.
This effectively changes the "correct usage" contract of createSetterMethod to be createNonDurationSetterMethod. Callers have to know about the second potential implementation and call the correct version -- which will either results in duplicated checks or a bug if someone doesn't realize.
Instead, consider relocating this check to be inside the createSetterMethod to maintain the simpler contract of "call this to get the correct setter method".
There was a problem hiding this comment.
Ah, that’s a good point! Currently the only call pattern is through createGetterSetters() since these are not exposed as public methods, but it will probably benefit future maintenance. I’ve moved the logic to all live inside createSetterMethod().
| .setValueExpr(parsedDurationExpr) | ||
| .build(); | ||
|
|
||
| String methodName = "set" + CaseFormat.LOWER_CAMEL.to(CaseFormat.UPPER_CAMEL, propertyName); |
There was a problem hiding this comment.
Is this equivalent?
String methodName = "set" + JavaStyle.toUpperCamelCase(propertyName);There was a problem hiding this comment.
Hmm good question - looks like under the hood JavaStyle.ToUpperCamelCase() uses CaseFormat to convert from lower snake to upper camel case, whereas we are converting from lower camel in this line.
I'm inclined to keep the current implementation but practically they do yield the same result, since here we just need the first letter of propertyName to change from lower to upper case.
| @@ -365,14 +394,8 @@ private static Map<String, TypeNode> createDynamicTypes(Service service, String | |||
| .setPakkage("org.springframework.boot.context.properties") | |||
| .build()); | |||
|
|
|||
| // import org.threeten.bp.Duration; | |||
There was a problem hiding this comment.
Thanks for catching this, it should be concrete type instead.
| Variable propertyVar = Variable.builder().setName(propertyName).setType(returnType).build(); | ||
| Expr thisExpr = ValueExpr.withValue(ThisObjectValue.withType(thisClassType)); | ||
| TypeNode oldDurationType = staticTypes.get("org.threeten.bp.Duration"); | ||
| TypeNode newDurationType = staticTypes.get("java.time.Duration"); |
There was a problem hiding this comment.
nit: perhaps naming as threetenDurationType and JavaDurationType?
There was a problem hiding this comment.
Yup - updated to threetenBpDurationType and javaTimeDurationType.
|
Kudos, SonarCloud Quality Gate passed! |
…tatic types instead of vapor references (#1046) Adding Spring related dependencies to build files and use static types instead of vapor references. Benefit of this change: - easier to test - less code change needed for future changes (e.g. package change of classes). One caveat is that because `gapic-generator-java` is brought into `googleapis` as `http_archive` code addition to `googleapis` is also needed to include these libraries. Add below code snippet to [WORKSPACE](https://github.com/googleapis/googleapis/blob/38e8b447d173909d3b2fe8fdc3e6cbb3c85442fd/WORKSPACE#L239-L245): ``` SPRING_MAVEN_ARTIFACTS = [ "org.springframework.boot:spring-boot-starter:2.7.4", "com.google.cloud:spring-cloud-gcp-core:3.3.0", ] maven_install( artifacts = PROTOBUF_MAVEN_ARTIFACTS + SPRING_MAVEN_ARTIFACTS, generate_compat_repositories = True, repositories = [ "https://repo.maven.apache.org/maven2/", ], ) ``` ------------ Updates: - Updated pr with #1065, - Moved annotation classed added in #1045 to static types. - Fixed conflicts from merged changes in #1093 - Moved logging classes added in #1053 to static types.
zhumin8
added
the
spring
This PR: updates setters for Spring properties of type
org.threeten.bp.Durationto accept argument of typejava.time.Durationinstead, and parse toorg.threeten.bp.Durationin the setter body.parse()andtoString()methods for both will accept or return ISO-8601 seconds based representation, such asPT8H6M12.345S.This addresses the issue that properties of type
org.threeten.bp.Durationcannot be set from strings (e.g. specified in Spring property files) because the implicit conversion is unsupported. Spring boot has dedicated support for properties of typejava.time.Duration.Note for review: would appreciate any feedback or thoughts on the conversion approach here! Some alternative approaches explored were:
java.time.Duration, but have getters modified to convert toorg.threeten.bp.Duration(poc)org.threeten.bp.Durationthrough@ConfigurationPropertiesBinding(poc)