fix(spring): update setters for properties of type Duration #1093
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| import com.google.api.generator.engine.ast.Expr; | ||
| import com.google.api.generator.engine.ast.ExprStatement; | ||
| import com.google.api.generator.engine.ast.MethodDefinition; | ||
| import com.google.api.generator.engine.ast.MethodInvocationExpr; | ||
| import com.google.api.generator.engine.ast.NewObjectExpr; | ||
| import com.google.api.generator.engine.ast.PrimitiveValue; | ||
| import com.google.api.generator.engine.ast.ScopeNode; | ||
|
|
@@ -256,7 +257,13 @@ private static List<MethodDefinition> createGetterSetters( | |
| String propertyName = Joiner.on("").join(methodAndPropertyName); | ||
| getterAndSetter.add( | ||
| createGetterMethod(thisClassType, propertyName, propertyType, null)); | ||
| if (propertyType.equals(staticTypes.get("org.threeten.bp.Duration"))) { | ||
| // Use different setter logic for Duration handling conversion | ||
| getterAndSetter.add( | ||
| createDurationSetterMethod(thisClassType, propertyName, propertyType)); | ||
| } else { | ||
| getterAndSetter.add(createSetterMethod(thisClassType, propertyName, propertyType)); | ||
| } | ||
| return getterAndSetter; | ||
| }, | ||
| (String propertyName) -> new ArrayList<>()); | ||
|
|
@@ -315,6 +322,52 @@ private static MethodDefinition createSetterMethod( | |
| .build(); | ||
| } | ||
|
|
||
| private static MethodDefinition createDurationSetterMethod( | ||
| TypeNode thisClassType, String propertyName, TypeNode returnType) { | ||
| Variable propertyVar = Variable.builder().setName(propertyName).setType(returnType).build(); | ||
| Variable argumentVar = | ||
| Variable.builder() | ||
| .setName(propertyName) | ||
| .setType(staticTypes.get("java.time.Duration")) | ||
| .build(); | ||
| Expr thisExpr = ValueExpr.withValue(ThisObjectValue.withType(thisClassType)); | ||
|
|
||
| MethodInvocationExpr durationToStringExpr = | ||
| MethodInvocationExpr.builder() | ||
| .setExprReferenceExpr(VariableExpr.withVariable(argumentVar)) | ||
| .setMethodName("toString") | ||
| .setReturnType(TypeNode.STRING) | ||
| .build(); | ||
|
|
||
| MethodInvocationExpr parsedDurationExpr = | ||
| MethodInvocationExpr.builder() | ||
| .setStaticReferenceType(staticTypes.get("org.threeten.bp.Duration")) | ||
| .setMethodName("parse") | ||
| .setArguments(durationToStringExpr) | ||
| .setReturnType(staticTypes.get("org.threeten.bp.Duration")) | ||
| .build(); | ||
|
|
||
| AssignmentExpr propertyVarExpr = | ||
| AssignmentExpr.builder() | ||
| .setVariableExpr( | ||
| VariableExpr.withVariable(propertyVar) | ||
| .toBuilder() | ||
| .setExprReferenceExpr(thisExpr) | ||
| .build()) | ||
| .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 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 |
||
|
|
||
| return MethodDefinition.builder() | ||
| .setName(methodName) | ||
| .setScope(ScopeNode.PUBLIC) | ||
| .setReturnType(TypeNode.VOID) | ||
| .setArguments(VariableExpr.builder().setVariable(argumentVar).setIsDecl(true).build()) | ||
| .setBody(Arrays.asList(ExprStatement.withExpr(propertyVarExpr))) | ||
| .build(); | ||
| } | ||
|
|
||
| private static Map<String, TypeNode> createDynamicTypes(Service service, String packageName) { | ||
| Map<String, TypeNode> typeMap = | ||
| Arrays.asList(CLASS_NAME_PATTERN).stream() | ||
|
|
@@ -365,14 +418,8 @@ private static Map<String, TypeNode> createDynamicTypes(Service service, String | |
| .setPakkage("org.springframework.boot.context.properties") | ||
| .build()); | ||
|
|
||
|
There was a problem hiding this comment. Thanks for catching this, it should be concrete type instead. |
||
| typeMap.put(service.name() + "Properties", clientProperties); | ||
| typeMap.put("Credentials", credentials); | ||
| typeMap.put("CredentialsSupplier", credentialsSupplier); | ||
| typeMap.put("ConfigurationProperties", configurationProperties); | ||
| typeMap.put("NestedConfigurationProperty", nestedConfigurationProperty); | ||
|
|
@@ -382,11 +429,11 @@ private static Map<String, TypeNode> createDynamicTypes(Service service, String | |
|
|
||
| private static Map<String, TypeNode> createStaticTypes() { | ||
| List<Class> concreteClazzes = | ||
| Arrays.asList( | ||
| RetrySettings.class, org.threeten.bp.Duration.class, java.time.Duration.class); | ||
| return concreteClazzes.stream() | ||
| .collect( | ||
| Collectors.toMap( | ||
| c -> c.getName(), c -> TypeNode.withReference(ConcreteReference.withClazz(c)))); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effectively changes the "correct usage" contract of
createSetterMethodto becreateNonDurationSetterMethod. 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
createSetterMethodto maintain the simpler contract of "call this to get the correct setter method".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 insidecreateSetterMethod().