Scope builder generation and builder usage by package (#114) - #26
devin-ai-integration[bot] wants to merge 15 commits into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
457817e to
aabccb0
Compare
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
| private static void setBuilderTypeIfAnnotated( | ||
| TypeName typeName, TypeElement typeElement, ProcessingContext context) { |
There was a problem hiding this comment.
I know this function has been there before. But is set... a good name here? Ok it is setting this type in typename, right? What would be used for name in other places? Is for example something with "apply" used there or even something else?
There was a problem hiding this comment.
Its siblings in JavaLangMapper are setBuilderAndConstructorInfo and setEmptyConstructorInfoIfAvailable, so set…If… is the local convention for "populate this TypeName property when condition holds"; apply/enhance are not used for this in the codebase (apply would collide with the Function.apply meaning). What was wrong is the condition: it is no longer "annotated" but "usable", so I renamed to setBuilderTypeIfUsable and setElementBuilderTypeIfUsable (9b8f4f7).
| @@ -216,22 +214,10 @@ private static void setBuilderAndConstructorInfo( | |||
| */ | |||
| private static void setBuilderTypeIfAnnotated( | |||
There was a problem hiding this comment.
Please check usage of this function! Is it repeated used on the same type? I am asking because the analysis is now much more complex then before!
There was a problem hiding this comment.
Checked: map2TypeName/map2MethodParameter are called once per field, parameter and generic argument, so the same referenced type is resolved as often as it is used in a DTO — before this PR that was also the case (two annotation lookups each time). What is new is the package match and, for the usage scope only, one Elements.getTypeElement lookup. In 9b8f4f7 the resolver now caches the Optional<TypeName> per referenced type FQN (computeIfAbsent), so each type is analysed once per target configuration; the parsed package sets are also cached and only re-read when initConfigurationForProcessingTarget hands over a different configuration. Test BuilderScopeResolverTest asserts the cached instance is returned (assertSame).
| context | ||
| .getBuilderScopeResolver() | ||
| .resolveUsableBuilderType(elementTypeElement, context) | ||
| .ifPresent(genericType::setElementBuilderType); |
There was a problem hiding this comment.
Same question here! How often is this executed and do we get a problem because it is now much more complex!?
There was a problem hiding this comment.
Same answer as above: the element type of every List<T>/Set<T> field goes through the same resolver, and the result is now cached per FQN in the resolver (9b8f4f7), so repeated collection fields with the same element type cost one lookup.
| * @return the package name (e.g., "com.example"), or empty string if no package | ||
| */ | ||
| private static String extractPackageName(String qualifiedName) { | ||
| static String extractPackageName(String qualifiedName) { |
There was a problem hiding this comment.
We have public it is needed elsewhere now? Maybe moving this function into a util class?
There was a problem hiding this comment.
extractPackageName is gone (9b8f4f7): both the resolver and createBuilderTypeName now use the existing ProcessingContext.getPackageName(Element) (Elements-based), which also yields the correct package for nested classes where the string split on the last dot did not.
| */ | ||
| private static TypeName createBuilderTypeName( | ||
| TypeElement typeElement, ProcessingContext context) { | ||
| static TypeName createBuilderTypeName(TypeElement typeElement, ProcessingContext context) { |
There was a problem hiding this comment.
Same here! Or why not having a public access modifier? In this project, protected modifiers are used seldom, right!?
There was a problem hiding this comment.
Made createBuilderTypeName public static (this project uses public for cross-class helpers, not package-private/protected) — it stays in JavaLangMapper because it produces a TypeName, which is what the mapper is for. Commit 9b8f4f7.
| .map(String::trim) | ||
| .filter(s -> !s.isEmpty()) |
There was a problem hiding this comment.
Why not filtering with StringUtils::isNoneBlank instead?
There was a problem hiding this comment.
Done, with StringUtils::isNotBlank — isNoneBlank is the varargs variant for several strings, isNotBlank is the single-argument predicate the stream needs. 9b8f4f7.
| */ | ||
| public void initConfigurationForProcessingTarget(BuilderConfiguration config) { | ||
| this.configurationForProcessingTarget = config; | ||
| this.builderScopeResolver = null; |
There was a problem hiding this comment.
Agreed — removed. The resolver is now created once in the constructor and holds no configuration state that must be reset; it reads context.getConfiguration() on use and refreshes its parsed scopes / result cache itself when the configuration changed (Objects.equals on the record). 9b8f4f7.
| if (builderScopeResolver == null) { | ||
| builderScopeResolver = new BuilderScopeResolver(this); | ||
| } |
There was a problem hiding this comment.
We need this resolver in many places, do like the other components in context, initialize in constructor!
There was a problem hiding this comment.
Done: private final BuilderScopeResolver builderScopeResolver initialised in the ProcessingContext constructor, plain getter. Because the target configuration is only known later (initConfigurationForProcessingTarget per DTO), the resolver reads the configuration lazily on each call and re-parses its scopes only when the configuration instance changed. 9b8f4f7.
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
…elpers#272) (java-helpers#281) * Adding a script for performance analysis and having multiple runs * Removing stability analysis because this is not needed in future * Adding a check for expected annotations on generated code so that analysis runs could not be done on different code-geration-runs before * Adding possibility to define formatting-mode when running performance-measurement * Fixing bugs in run_performance_measurements * adding script for running a full analysis with comparing the results of all builder types * Removing stability analysis because this is not needed anymore * Updating performance analysis documentation * Optimization in run_full_analysis for usage with non-mac-os systems too
…a-helpers#280) Bumps the maven-plugins group with 1 update in the /core directory: [org.apache.maven.plugins:maven-compiler-plugin](https://github.com/apache/maven-compiler-plugin). Bumps the maven-plugins group with 1 update in the /example directory: [org.apache.maven.plugins:maven-compiler-plugin](https://github.com/apache/maven-compiler-plugin). Bumps the maven-plugins group with 1 update in the /example-custom-generator directory: [org.apache.maven.plugins:maven-compiler-plugin](https://github.com/apache/maven-compiler-plugin). Bumps the maven-plugins group with 2 updates in the /processor directory: [org.apache.maven.plugins:maven-compiler-plugin](https://github.com/apache/maven-compiler-plugin) and [org.apache.maven.plugins:maven-surefire-plugin](https://github.com/apache/maven-surefire). Updates `org.apache.maven.plugins:maven-compiler-plugin` from 3.15.0 to 3.16.0 - [Release notes](https://github.com/apache/maven-compiler-plugin/releases) - [Commits](apache/maven-compiler-plugin@maven-compiler-plugin-3.15.0...maven-compiler-plugin-3.16.0) Updates `org.apache.maven.plugins:maven-compiler-plugin` from 3.15.0 to 3.16.0 - [Release notes](https://github.com/apache/maven-compiler-plugin/releases) - [Commits](apache/maven-compiler-plugin@maven-compiler-plugin-3.15.0...maven-compiler-plugin-3.16.0) Updates `org.apache.maven.plugins:maven-compiler-plugin` from 3.15.0 to 3.16.0 - [Release notes](https://github.com/apache/maven-compiler-plugin/releases) - [Commits](apache/maven-compiler-plugin@maven-compiler-plugin-3.15.0...maven-compiler-plugin-3.16.0) Updates `org.apache.maven.plugins:maven-compiler-plugin` from 3.15.0 to 3.16.0 - [Release notes](https://github.com/apache/maven-compiler-plugin/releases) - [Commits](apache/maven-compiler-plugin@maven-compiler-plugin-3.15.0...maven-compiler-plugin-3.16.0) Updates `org.apache.maven.plugins:maven-surefire-plugin` from 3.5.6 to 3.6.0 - [Release notes](https://github.com/apache/maven-surefire/releases) - [Commits](apache/maven-surefire@surefire-3.5.6...surefire-3.6.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-compiler-plugin dependency-version: 3.16.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: maven-plugins - dependency-name: org.apache.maven.plugins:maven-compiler-plugin dependency-version: 3.16.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: maven-plugins - dependency-name: org.apache.maven.plugins:maven-compiler-plugin dependency-version: 3.16.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: maven-plugins - dependency-name: org.apache.maven.plugins:maven-compiler-plugin dependency-version: 3.16.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: maven-plugins - dependency-name: org.apache.maven.plugins:maven-surefire-plugin dependency-version: 3.6.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: maven-plugins ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…elpers#286) The action only executes the scanner on the runner — use of an unmodified tool, not distribution — so LGPL obligations do not apply. Scope the exception via allow-dependencies-licenses so the fail-closed allow-licenses list stays strict for all other dependencies. Generated with [Devin](https://devin.ai) Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Stop executing fork-controlled Maven in the privileged fork Sonar workflow (java-helpers#232) (java-helpers#285) The fork Sonar job ran `mvn sonar:sonar` on the checked-out fork tree with SONAR_TOKEN set — arbitrary code execution via fork-controlled pom.xml/.mvn/wrapper in a privileged context. Now nothing fork-controlled is executed: Maven only runs on the trusted base checkout (dependency classpath), the fork head is fetched as git objects and verified against the CI-built SHA, and analysis uses the standalone sonarqube-scan-action with workflow-written config passed via project.settings — so fork pom/properties files are inert. The fork-ci environment gate remains as defense-in-depth.
…rs#114) Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
| */ | ||
| public static boolean isInScope(String packageName, Set<String> packageScopes) { | ||
| for (String scope : packageScopes) { | ||
| if (StringUtils.equalsIgnoreCase(packageName, scope) |
| public static boolean isInScope(String packageName, Set<String> packageScopes) { | ||
| for (String scope : packageScopes) { | ||
| if (StringUtils.equalsIgnoreCase(packageName, scope) | ||
| || StringUtils.startsWithIgnoreCase(packageName, scope + ".")) { |
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
…a-helpers#114) Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
Upstream split the completeness test into buildFullyConfigured() helper; kept the refactor and added the new builder scoping options to it.
StringUtils.equalsIgnoreCase/startsWithIgnoreCase are deprecated in commons-lang3 in favor of Strings.CI.
|
moved to java-helpers#288 |
Summary
Previously the processor emitted a builder reference for every
@SimpleBuilder-annotated field/collection-element type. That breaks when the referenced type is a precompiled library DTO whose builder was never generated: the generated code references a class that does not exist.Two comma-separated package-list options now scope this, and the "may a builder be used here?" decision is centralized in one new class so generators/enhancers stay ignorant of the mechanism — they keep checking only
TypeName.getBuilderType().isPresent().builderGenerationPackages— when non-empty,BuilderProcessoronly generates builders for annotated DTOs in scope. In-scope types are trusted, so their builder is referenced without a type search.builderUsagePackages— packages whose builders may be used as helpers. In usage scope but outside generation scope ⇒ the builder must be verified via type lookup before a reference is emitted; unresolvable ⇒ the field falls back to a plain setter.Decision logic, all in
BuilderScopeResolver(analysis/), created once in theProcessingContextconstructor (getBuilderScopeResolver()); it reads the current target configuration on use, re-parses its package scopes only when that configuration changes, and caches the per-type result (Optional<TypeName>by FQN) so each referenced type is analysed once per configuration:generatedThisRoundcovers the case "generation scope unset, usage scope set": every annotated DTO is then generated, but a nested DTO in the usage scope cannot be found via type lookup because its builder is being generated in the same round.BuilderProcessortherefore resolves configuration and applies the generation filter for all elements first, registers the survivors viaBuilderScopeResolver.registerGeneratedTypes(...), and only then generates. Types in neither scope stay unreferenced even when generated in the same round.Opt-out precedence is structural rather than a separate rule: keying on the same annotation/opt-out check that drives generation means a DTO that will not get a builder can never be referenced, even when both scopes include its package. The previous
@Ignore4BuilderGenerationcheck inJavaLangMapperwas folded into the resolver so there is a single decision point; the mapper now only doesresolveUsableBuilderType(...).ifPresent(typeName::setBuilderType)(and the collection-element equivalent).Matching is case-insensitive and package-boundary aware —
StringUtils.equalsIgnoreCase(pkg, p) || StringUtils.startsWithIgnoreCase(pkg, p + ".")(soa.bcdoes not match scopea.b, whileCOM.Examplematchescom.example) — implemented once asBuilderConfiguration.isInScope(...)and shared by the resolver andBuilderProcessor. Comma lists are parsed likeComponentFilter.parsePatterns(StringUtils.split(value, ",")+ trim; blank ⇒ empty set ⇒ unscoped).Wiring is the usual three layers:
SimpleBuilder.Options,CompilerArgumentsEnum/CompilerArgumentsReader(-Asimplebuilder.builderGenerationPackages,-Asimplebuilder.builderUsagePackages),BuilderConfigurationReader.parseOptionsFromMirror, andBuilderConfiguration(record field,DEFAULT, builder setter viatrimToNull,merge,toString, getters).Example module
A runnable demo lives in
org.javahelpers.simple.builders.example.scoping, with the generated result committed underexample/generated-example-builder/.ScopedOwnerDtoconfigures both scopes inline and has three fields covering all three outcomes:ScopedOwnerDtoBuildertrustedtrusted(Consumer<TrustedHelperDtoBuilder>)emittedlibrarysponsorSponsorDtoBuilderexistsLibraryHelperDtois annotated but configured outside the generation scope, so no builder is generated for it — that is how a precompiled library DTO without a builder is simulated inside one compilation.Note: the
examplemodule's tests are compiled but never executed by Maven (the reactor inherits Surefire 2.12.4 and the module has no JUnit 5 engine), soScopedOwnerDtoBuilderTestis compile-checked in CI and was run out-of-band via the JUnit Platform launcher (2/2 passing). This is pre-existing and affects the module's other test classes too.Tests
BuilderScopeProcessingTestcovers the compile-level behavior: both options unset (regression), exact and subpackage generation matching, out-of-scope DTOs getting no builder, usage scope with an existing vs. missing builder, a same-round helper with only the usage scope set, opt-out precedence, and both the compiler-arg and inline@SimpleBuilder(options = ...)forms.BuilderConfigurationTestcovers comma-splitting/trimming, blank ⇒ unscoped, and exact-vs-subpackage matching;BuilderScopeResolverTestcovers the plain-setter fallback, configuration changes on the same resolver instance and result caching. Processor suite: 432 tests green, full reactormvn clean verifygreen.Docs
docs/CONFIGURATION.mdgets a "Builder Scoping" subsection plus entries in the compiler-option and complete-example references;docs/CUSTOMIZING.mdnotes that custom generators/enhancers need no changes;README.mdgets a "Builder Scoping Example" section linking the demo files in the same style as the other example sections.Link to Devin session: https://app.devin.ai/sessions/d691231fba0842a4b842b275ca4b35ed
Open in Devin Desktop: https://app.devin.ai/desktop/session/d691231fba0842a4b842b275ca4b35ed?variant=devin
Requested by: @AndreasIgel