Spring Boot properties extension #6269
Conversation
|
I hope to be able to review this tomorrow but I can't promise :) |
| "artifact-id": "quarkus-spring-boot-properties", | ||
| "version": "999-SNAPSHOT", | ||
| "description": "Use Spring Boot properties annotations to configure our application" | ||
| }, |
There was a problem hiding this comment.
OK I'll remove it. I saw other spring modules mentioned in this file so I followed the trend.
|
@gytis did you check why CI is failing? |
gytis
force-pushed
the
spring-boot-properties
branch
from
bf2ec0a
to
1a33008
Compare
I think it failed because of |
.../main/java/io/quarkus/arc/deployment/configproperties/ConfigPropertiesMetadataBuildItem.java
Show resolved
Hide resolved
| @@ -35,17 +26,26 @@ | |||
|
|
|||
| public class ConfigPropertiesBuildStep { | |||
|
|
|||
| @BuildStep | |||
| void produceConfigPropertiesMetadata(CombinedIndexBuildItem combinedIndex, | |||
There was a problem hiding this comment.
This is a really minor complaint, but the rest of the class doesn't use lambda's so can we keep it consistent please?
There was a problem hiding this comment.
OK will do. But I still think that streams are more readable :)
There was a problem hiding this comment.
I used to think the same :). Let's keep the streams in the other uses since those are for new classes
There was a problem hiding this comment.
Disillusionment :). I liked them because they were cool, then once I stepped back from them for a while I started disliking them and find that I need to think more about what they are doing
There was a problem hiding this comment.
I sort of agree with simple cases, like the one in this method. Both stream and loop are easy to read in this case. But if it would be a method with a bunch of it/else and loops, I don't think it would more readable than stream with lambdas.
| return new ConfigPropertiesMetadataBuildItem(getReturnClassInfo(index, annotation), getPrefix(annotation)); | ||
| } | ||
|
|
||
| private ClassInfo getReturnClassInfo(IndexView index, AnnotationInstance annotation) { |
There was a problem hiding this comment.
I would just inline this method since it's only called at one spot and more importantly it can only work when the target is a method
| public class InterfacePropertiesResource { | ||
|
|
||
| @Inject | ||
| private InterfaceProperties properties; |
There was a problem hiding this comment.
Can we drop private from here (and the other instances in the integration test application)? The reason is that quarkus logs a warning for injections into private fields and we could use the cleanest test output we can get.
There was a problem hiding this comment.
Didn't know that. Why is package-private preferred to private?
There was a problem hiding this comment.
Because Quarkus then doesn't need to use reflection to set the field. You can read more details here: https://quarkus.io/guides/cdi-reference#native-executables-and-private-members
gytis
force-pushed
the
spring-boot-properties
branch
from
1a33008
to
56b2730
Compare
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-core-deployment</artifactId> |
There was a problem hiding this comment.
This isn't necessary because quarkus-arc-deployment brings it in
| @@ -0,0 +1,12 @@ | |||
| ### Current differences with Spring Boot properties. | |||
|
|
|||
| 1. Default values assigned with `=` operator are not supported. | |||
There was a problem hiding this comment.
My bad, this readme wasn't supposed to be in this PR and is not fully correct. I'll remove it. = operator works for Spring Boot properties
| ### Current differences with Spring Boot properties. | ||
|
|
||
| 1. Default values assigned with `=` operator are not supported. | ||
| 2. Getters and setters are needed for all fields, including nested classes. |
There was a problem hiding this comment.
Yes. I tried the Spring Boot way where a config field of a pojo type has only a getter and I ended up getting Quarkus extension that the field doesn't have a setter
|
|
||
| 1. Default values assigned with `=` operator are not supported. | ||
| 2. Getters and setters are needed for all fields, including nested classes. | ||
| 3. `@Bean` needs to be removed from a method which is annotated with `@ConfigurationProperties`. |
There was a problem hiding this comment.
It shouldn't be hard to make this work - basically by just ignoring @Bean. We should follow up this PR with that
| Otherwise two beans will be created and injection will fail. | ||
| 4. `@NestedConfigurationProperty` can be used but is not required. | ||
| Nested property classes work out of the box with inner and outer classes. | ||
| 5. `@ConstructorBinding` is not supported. |
There was a problem hiding this comment.
This would be a nice follow up that would also work for Quarkus's @ConfigProperties.
| Nested property classes work out of the box with inner and outer classes. | ||
| 5. `@ConstructorBinding` is not supported. | ||
| 6. Spring Boot properties conversion is not supported. | ||
| 7. Random values are not supported |
There was a problem hiding this comment.
Spring Boot properties support various expressions in application.properties. One of them are random values where you can set something like this: my.number=${random.int}. Of course, if it's needed it should be a feature of Quarkus properties rather than Spring Boot properties extension
gytis
force-pushed
the
spring-boot-properties
branch
from
56b2730
to
cc000e9
Compare
There was a problem hiding this comment.
Actually, the CI config needs to be updated as well: https://github.com/quarkusio/quarkus/blob/master/ci-templates/stages.yml#L362
gytis
force-pushed
the
spring-boot-properties
branch
from
cc000e9
to
bfeb1ee
Compare
|
It's just the (integration test) directory name that needs to be added |
|
does this thing do what I think it does ? adding a quarkus-springboot-properties extension that will pickup springboot keys in application.properties or ? |
|
@maxandersen no. This allows users to use Spring Boot's |
|
I have one question about that one: we created an extension that specifically introduces support for Spring Boot properties. What if we want to introduce more features? We will create one extension per feature? That doesn't seem ideal. |
|
Any ideas on what to call it? |
|
Well, I don't even know if you plan to add more features? But if so, it would probably be "spring-boot" instead of having 5 different extensions with different features. But I'm not totally sure what our target is here, just raising the issue. |
|
I see your point and I do have the same concerns. However, naming the extension spring-boot would probably lead to massive confusion and a ton of issues being opened asking why this or that spring Boot feature doesn't work. |
|
Maybe we could merge the extensions in the future if the number becomes too big? At the end of the day all the Spring and Spring Boot features are marked as preview. However, merging this extension with Spring DI would not be an ideal scenario IMO. The properties API that is used in this extension comes from Spring Boot project directly. Spring DI on the other had is a separate project and has a much smaller scope. |
+1 |
|
I propose that for the time being we leave the name as is and leave it in preview state, that way we can change it later on if needed |
|
OK, I suppose I can live with that :). |
cc @geoand