Upgrade dependencies liquibase to 4.29.1 and liquibase-mongodb to 4.28.0 #42308

gcw-it · 2024-08-04T11:17:34Z

The liquibase dependencies are upgraded to their latest versions.

To create an easier upgrade path for future version upgrades, a utility class to define resources limited to a single dependency is introduced. The functionality added in PR #41928 is extracted to a helper class, to be used in selected build steps. An option is added to NativeImageResourceBuildItem and ServiceProviderBuildItem to make use of this new capability.

There is some possible overlap with NativeImageResourceDirectoryBuildItem and NativeImageResourcePatternsBuildItem, but the new feature offers to reduce the scope to a single dependency instead of the whole classpath.

This is desirable in view of Liquibases's decision to put .property files in the root of their .jar. This could lead to possible conflicts, when using e.g. NativeImageResourcePatternsBuildItem to include those resources., should other dependency providers choose to go down a similar road, and put their resources in the same namespace.

Furthermore it enables an extension to include all SPIs defined in META-INF/services of a specific dependency.

gsmet

This looks really good thanks. I added a minor nitpick for the future but it's no biggie, we can merge as is.
If you end up fixing it, please amend the commit in which this thing was introduced (for instance using git commit --fixup <sha> and then git rebase -i <sha before the first commit> --autosquash).

gsmet · 2024-08-07T07:46:28Z

core/deployment/src/main/java/io/quarkus/deployment/util/ServiceUtil.java

@@ -19,6 +20,8 @@
 /**
 */
 public final class ServiceUtil {
+    private static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8;


I'm not going to block the PR for this but this is not really an improvement. It's already a constant and now to figure out the charset you have to look out for the constant.

gcw-it · 2024-08-07T10:34:25Z

I fixed a small error, I've overlooked.

gsmet · 2024-08-07T10:48:42Z

...loyment/src/main/java/io/quarkus/liquibase/mongodb/deployment/LiquibaseMongodbProcessor.java

-            }
+        } catch (Exception ex) {
+            // close() really shouldn't declare that exception, see also https://github.com/liquibase/liquibase/pull/2576
+            throw new AssertionError(ex);


Sorry I missed this but I think we need something better here.

Because the exception is caught globally not only for the close() now and it could happen in the code that is in the try. So I would rather have an IllegalStateException with a proper message and the cause.

I changed the exception accordingly

gastaldi · 2024-08-07T13:54:04Z

/cc @aloubyansky for the core improvements

aloubyansky · 2024-08-07T14:19:52Z

core/deployment/src/main/java/io/quarkus/deployment/util/ArtifactResourceResolver.java

+                return pathStream
+                        .map(p -> p.subpath(0, p.getNameCount()))
+                        .filter(p -> p.toString().endsWith(resourceCoordinates.extension()))
+                        .toList();


These paths, theoretically, might not be usable outside the method they are collected in.
They will work as long as the path tree is backed by a regular directory or an archive who's FileSystem remains open.

replaced with filtered pathTree.walk

aloubyansky · 2024-08-07T14:22:01Z

core/deployment/src/main/java/io/quarkus/deployment/util/ArtifactResourceResolver.java

+                .toList();
+    }
+
+    public record ArtifactCoordinates(String groupId, String artifactId) {


Let's avoid introducing this API. We already have a few variations around and this record does not represent complete coordinates. Consider https://github.com/quarkusio/quarkus/blob/main/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/maven/dependency/ArtifactCoordsPattern.java

Thanks for the pointer. Done.

aloubyansky · 2024-08-07T14:25:24Z

core/deployment/src/main/java/io/quarkus/deployment/util/ArtifactResourceResolver.java

+        }
+    }
+
+    public record ResourceCoordinates(String basePath, String extension) {


This also looks questionable to me. It seems to be an attempt to introduce a generic way of locating content but the way it works remains pretty specific. Have you considered https://github.com/quarkusio/quarkus/blob/main/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/PathFilter.java?

Thanks for the pointer. Done.

aloubyansky

Looks better to me, thanks @gcw-it

aloubyansky · 2024-08-08T08:24:49Z

...loyment/src/main/java/io/quarkus/liquibase/mongodb/deployment/LiquibaseMongodbProcessor.java

-                "www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd",
-                "www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd",
-                "liquibase.build.properties"));
+        var dependencies = curateOutcome.getApplicationModel().getDependencies();


AFAICS, there are two places where the new ofDependencyResources method is used.

Just to clarify, are all dependencies instead of only the runtime ones scanned on purpose in both cases?

You're right, we could reduce the scope to the runtime dependencies. I'll take care of that.

aloubyansky · 2024-08-08T08:36:48Z

...loyment/src/main/java/io/quarkus/liquibase/mongodb/deployment/LiquibaseMongodbProcessor.java

+        resource.produce(NativeImageResourceBuildItem.ofDependencyResources(
+                dependencies, LIQUIBASE_MONGODB_ARTIFACT, LIQUIBASE_MONGODB_CHANGELOG_FILTER));
+        resource.produce(NativeImageResourceBuildItem.ofDependencyResources(
+                dependencies, LIQUIBASE_MONGODB_ARTIFACT, LIQUIBASE_MONGODB_PARSER_FILTER));


It seems like it could have been somewhat simplified by combining all the filters applied to the same set of of artifacts into one. I mean PathFilter.forIncludes(List.of(...)) could have been used instead, which would result in two calls to resource.produce(NativeImageResourceBuildItem.ofDependencyResources( here and one in the other place.

Then, personally, it doesn't seem like the new NativeImageResourceBuildItem.ofDependencyResources() offers a lot of benefit, since essentially, it wraps a one-liner ArtifactResourceResolver.of(dependencies, artifactCoordinates).resourceList(resourceFilter) and by that, from the caller's perspective, it takes the responsibility of filtering resources off of the ArtifactResourceResolver. I'll leave that decision to you though.

I'll combine the resource filters. That was something I thought about during the modification and then promptly forgot about it. Thanks for the reminder.

I introduced the NativeImageResourceBuildItem.ofDependencyResources convenience method, to be on the same abstraction level as ServiceProviderBuildItem allProvidersOfDependency. It's where I would look first, to determine which capabilities are offered. In my opinion using the ArtifactResourceResolver is a bit more obscure.

I would keep the method, but since you have the better domain knowledge, I will remove it, if you think it's cleaner to use the ArtifactResourceResolver directly. What do you think?

Sure, feel free to keep the method. Thanks.

aloubyansky · 2024-08-08T09:44:28Z

...ment/src/main/java/io/quarkus/deployment/builditem/nativeimage/ServiceProviderBuildItem.java

+     */
+    public static Collection<ServiceProviderBuildItem> allProvidersOfDependency(
+            Collection<ResolvedDependency> dependencies,
+            ArtifactCoords artifactCoordinates) {


BTW, you could take a similar approach here by adding a method that takes ArtifactCoordsPattern and have a single call when it's called one per coords. That could be more efficient.

Or accept a List of ArtifactCoords, I think patterns would make more sense though, since that's how they are treated anyway.

Do you mean using a wildcard pattern, or rather a list of ArtifactCoordsPattern to include multiple dependencies?

Not a list of ArtifactCoordsPatterns. It should be either a single ArtifactCoordsPattern or a Collection<ArtifactCoords> from which a pattern instance will be created.

If you go with the Collection<ArtifactCoords> then I suppose you could keep the current method, in case there are callers that pass in a single coords.

Sorry, if I don't follow you completely, but how would I build an ArtifactCoordsPattern for multiple artifacts?
Using a wildcard pattern for the group- and/or artifact-id could lead to including unwanted resources. Am I misunderstanding anything here?

I thought of ArtifactCoordsPattern. toPatterns(Collection coords)

I see. I misunderstood your earlier comment

Not a list of ArtifactCoordsPatterns

That's why I disregarded this method.
Thanks for the support. I'll have a go at it.

…eMongodbProcessor

gcw-it · 2024-08-08T13:53:13Z

artifact scope îs reduced to runtime artifacts
the resource filters are combined
the ServiceProviderBuildItem provides now a method to get the service providers for a collection of artifacts

quarkus-bot · 2024-08-08T19:39:14Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit a9fcc8d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

aloubyansky

Thanks for your efforts @gcw-it

gsmet · 2024-08-08T20:40:17Z

And merged, thanks a lot!

gcw-it · 2024-08-08T21:10:35Z

@aloubyansky @gsmet Happy to help.

quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file area/liquibase labels Aug 4, 2024

gcw-it mentioned this pull request Aug 4, 2024

Bump org.liquibase:liquibase-core from 4.27.0 to 4.29.1 #42257

Closed

gsmet approved these changes Aug 7, 2024

View reviewed changes

gcw-it force-pushed the pr_lbupg branch 2 times, most recently from 4507e7c to 8da35f6 Compare August 7, 2024 10:32

gsmet reviewed Aug 7, 2024

View reviewed changes

gcw-it force-pushed the pr_lbupg branch from 8da35f6 to 1d946ed Compare August 7, 2024 11:19

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Aug 7, 2024

gastaldi requested a review from aloubyansky August 7, 2024 13:53

aloubyansky reviewed Aug 7, 2024

View reviewed changes

gcw-it force-pushed the pr_lbupg branch from 1d946ed to 2df8854 Compare August 8, 2024 06:22

aloubyansky reviewed Aug 8, 2024

View reviewed changes

gcw-it added 3 commits August 8, 2024 15:47

created mechanism that allows adding resources of selected dependencies

d2289a5

use dependency resource mechanism for LiquibaseProcessor and Liquibas…

fa2501c

…eMongodbProcessor

upgrade liquibase to 4.29.1 and liquibase-mongodb to 4.28.0

a9fcc8d

gcw-it force-pushed the pr_lbupg branch from 2df8854 to a9fcc8d Compare August 8, 2024 13:47

aloubyansky approved these changes Aug 8, 2024

View reviewed changes

gsmet merged commit 4ad4bba into quarkusio:main Aug 8, 2024
52 checks passed

quarkus-bot bot added this to the 3.14 - main milestone Aug 8, 2024

gcw-it deleted the pr_lbupg branch August 8, 2024 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade dependencies liquibase to 4.29.1 and liquibase-mongodb to 4.28.0 #42308

Upgrade dependencies liquibase to 4.29.1 and liquibase-mongodb to 4.28.0 #42308

gcw-it commented Aug 4, 2024 •

edited

Loading

gsmet left a comment

gsmet Aug 7, 2024

gcw-it Aug 7, 2024

gcw-it commented Aug 7, 2024

gsmet Aug 7, 2024

gcw-it Aug 7, 2024

This comment has been minimized.

gastaldi commented Aug 7, 2024

aloubyansky Aug 7, 2024

gcw-it Aug 8, 2024

aloubyansky Aug 7, 2024

gcw-it Aug 8, 2024

aloubyansky Aug 7, 2024

gcw-it Aug 8, 2024

aloubyansky left a comment

aloubyansky Aug 8, 2024

gcw-it Aug 8, 2024

aloubyansky Aug 8, 2024

gcw-it Aug 8, 2024 •

edited

Loading

aloubyansky Aug 8, 2024

aloubyansky Aug 8, 2024

aloubyansky Aug 8, 2024

gcw-it Aug 8, 2024

aloubyansky Aug 8, 2024

aloubyansky Aug 8, 2024

gcw-it Aug 8, 2024

aloubyansky Aug 8, 2024

gcw-it Aug 8, 2024 •

edited

Loading

gcw-it commented Aug 8, 2024

quarkus-bot bot commented Aug 8, 2024 •

edited by github-actions bot

Loading

aloubyansky left a comment

gsmet commented Aug 8, 2024

gcw-it commented Aug 8, 2024

Upgrade dependencies liquibase to 4.29.1 and liquibase-mongodb to 4.28.0 #42308

Upgrade dependencies liquibase to 4.29.1 and liquibase-mongodb to 4.28.0 #42308

Conversation

gcw-it commented Aug 4, 2024 • edited Loading

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcw-it commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

gastaldi commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloubyansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcw-it Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcw-it Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

gcw-it commented Aug 8, 2024

quarkus-bot bot commented Aug 8, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

aloubyansky left a comment

Choose a reason for hiding this comment

gsmet commented Aug 8, 2024

gcw-it commented Aug 8, 2024

gcw-it commented Aug 4, 2024 •

edited

Loading

gcw-it Aug 8, 2024 •

edited

Loading

gcw-it Aug 8, 2024 •

edited

Loading

quarkus-bot bot commented Aug 8, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`