Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #2169] Allow filtering over processInstances.definition and definition.metadata #2191

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Feb 7, 2025

Fix #2169

The Graphql schema already declared metadata as json.
Therefore we had two options to add query support in postgresql:

  • Change the graphql shema and define a new Metadata object, consisting of a list of name-value pairs
  • Keep the grapgql schema as Json and reuse the code added to support random queries over ProcessInstances.variables (also declared as Json)

I chose the latter because it give more freedom over what can be stored in metadata and keep the current graphql interface unchanged. The main cons is that it required a DB schema change for JPA (but that is handled by the flyway scripts automatically, so not a big deal, in my opinion)

@fjtirado fjtirado marked this pull request as draft February 7, 2025 17:56
@fjtirado fjtirado force-pushed the Fix_#2170 branch 3 times, most recently from 33d634c to 1babdd1 Compare February 7, 2025 18:18
@fjtirado fjtirado changed the title Fix #2170 Fix #2169 Allow filtering over processInstances.definition and definition.metadata Feb 7, 2025
@fjtirado fjtirado force-pushed the Fix_#2170 branch 2 times, most recently from 8f57738 to 1e7c3e1 Compare February 11, 2025 19:57
@fjtirado fjtirado force-pushed the Fix_#2170 branch 2 times, most recently from 905bffc to 6a1f2b3 Compare February 12, 2025 16:15
@apache apache deleted a comment from kie-ci3 Feb 12, 2025
@apache apache deleted a comment from kie-ci3 Feb 12, 2025
@apache apache deleted a comment from kie-ci3 Feb 12, 2025
@kie-ci3
Copy link
Contributor

kie-ci3 commented Feb 12, 2025

PR job #13 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-apps -u #2191 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-apps-pr/job/PR-2191/13/display/redirect

Test results:

  • PASSED: 1816
  • FAILED: 2

Those are the test failures:

PR check / Build projects / org.kie.kogito.addons.quarkus.data.index.it.MongoQuarkusAddonDataIndexIT.testDataIndexAddon java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/mongo:7.0
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.startMongo(DevServicesMongoProcessor.java:122)
at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
at java.base/java.lang.Thread.run(Thread.java:840)
at org.jboss.threads.JBossThread.run(JBossThread.java:483)
Caused by: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/mongo:7.0
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:359)
at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:330)
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.lambda$startMongo$0(DevServicesMongoProcessor.java:191)
at java.base/java.util.Optional.orElseGet(Optional.java:364)
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.startMongo(DevServicesMongoProcessor.java:208)
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.startMongo(DevServicesMongoProcessor.java:112)
... 9 more
Caused by: org.rnorth.ducttape.RetryCountExceededException: Retry limit hit with exception
at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:88)
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:344)
... 14 more
Caused by: org.testcontainers.containers.ContainerLaunchException: Could not create/start container
at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:563)
at org.testcontainers.containers.GenericContainer.lambda$doStart$0(GenericContainer.java:354)
at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:81)
... 15 more
Caused by: java.lang.IllegalStateException: Wait strategy failed. Container exited with code 132
at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:533)
... 17 more
Caused by: org.testcontainers.containers.ContainerLaunchException: Timed out waiting for log output matching '(?i).*waiting for connections.*'
at org.testcontainers.containers.wait.strategy.LogMessageWaitStrategy.waitUntilReady(LogMessageWaitStrategy.java:47)
at org.testcontainers.containers.wait.strategy.AbstractWaitStrategy.waitUntilReady(AbstractWaitStrategy.java:52)
at org.testcontainers.containers.GenericContainer.waitUntilContainerStarted(GenericContainer.java:909)
at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:500)
... 17 more
PR check / Build projects / org.kie.kogito.addons.quarkus.data.index.it.MongoQuarkusAddonDataIndexIT.testDataIndexAddon java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/mongo:7.0
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.startMongo(DevServicesMongoProcessor.java:122)
at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
at java.base/java.lang.Thread.run(Thread.java:840)
at org.jboss.threads.JBossThread.run(JBossThread.java:483)
Caused by: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/mongo:7.0
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:359)
at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:330)
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.lambda$startMongo$0(DevServicesMongoProcessor.java:191)
at java.base/java.util.Optional.orElseGet(Optional.java:364)
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.startMongo(DevServicesMongoProcessor.java:208)
at io.quarkus.mongodb.deployment.DevServicesMongoProcessor.startMongo(DevServicesMongoProcessor.java:112)
... 9 more
Caused by: org.rnorth.ducttape.RetryCountExceededException: Retry limit hit with exception
at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:88)
at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:344)
... 14 more
Caused by: org.testcontainers.containers.ContainerLaunchException: Could not create/start container
at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:563)
at org.testcontainers.containers.GenericContainer.lambda$doStart$0(GenericContainer.java:354)
at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:81)
... 15 more
Caused by: java.lang.IllegalStateException: Wait strategy failed. Container exited with code 132
at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:533)
... 17 more
Caused by: org.testcontainers.containers.ContainerLaunchException: Timed out waiting for log output matching '(?i).*waiting for connections.*'
at org.testcontainers.containers.wait.strategy.LogMessageWaitStrategy.waitUntilReady(LogMessageWaitStrategy.java:47)
at org.testcontainers.containers.wait.strategy.AbstractWaitStrategy.waitUntilReady(AbstractWaitStrategy.java:52)
at org.testcontainers.containers.GenericContainer.waitUntilContainerStarted(GenericContainer.java:909)
at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:500)
... 17 more

@fjtirado fjtirado marked this pull request as ready for review February 12, 2025 20:10
*/

ALTER TABLE definitions ADD COLUMN metadata varchar(max);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we need data migration for metedata when using "ansi" JPA

Copy link
Contributor Author

@fjtirado fjtirado Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented deletion of the old data and added how the scripts to migrate data should look like for different dbs (in case users want to apply manually)

@@ -126,11 +126,11 @@ public void setAnnotations(Set<String> annotations) {
this.annotations = annotations;
}

public Map<String, String> getMetadata() {
public Map<String, Object> getMetadata() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only change in the model.
Moving from string to object allow us to use something different than a String (currently only supported by postgresql) and keep other Dbs basically unchanged.

@@ -61,7 +62,7 @@ public ProcessDefinition readFrom(ProtoStreamReader reader) throws IOException {
pd.setName(reader.readString(NAME));
pd.setDescription(reader.readString(DESCRIPTION));
pd.setAnnotations(reader.readCollection(ANNOTATIONS, new HashSet<>(), String.class));
pd.setMetadata(buildMetadata(reader));
pd.setMetadata(Collections.unmodifiableMap(buildMetadata(reader)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a compilation trick to reuse the Map<String,String> stored in Hibernate as Map<String,Object> as required by the model change

@@ -148,11 +148,11 @@ public void setAnnotations(Set<String> annotations) {
this.annotations = annotations;
}

public Map<String, String> getMetadata() {
public Map<String, Object> getMetadata() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MongoDb should handle the Map<String,Object> in a similar way than the Map<String,String>, but it will be good to test this manually with an existing mongodb table

Copy link
Contributor Author

@fjtirado fjtirado Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it works by performing a quick test

public class MongoDBTest {
    public static void main(String[] args) {
        String uri = "mongodb://localhost";
        try (MongoClient mongoClient = MongoClients.create(uri)) {
            MongoDatabase database = mongoClient.getDatabase("sample_mflix");
            MongoCollection<Document> collection = database.getCollection("movies");
            
           // Map<String,String> map = Map.of("name","pepe");
           // Document insert = new Document();
            //insert.put("person", map);
            //collection.insertOne(new Document(insert));
            Document doc = collection.find(eq("person.name", "pepe")).first();
            Map<String,Object> map = (Map<String, Object>) doc.get("person");
            if (doc != null) {
                System.out.println(map);
            } else {
                System.out.println("No matching documents found.");
            }
        }
    }
}

@fjtirado fjtirado changed the title Fix #2169 Allow filtering over processInstances.definition and definition.metadata [Fix #2169] Allow filtering over processInstances.definition and definition.metadata Feb 14, 2025
@kie-ci3
Copy link
Contributor

kie-ci3 commented Feb 14, 2025

PR job #18 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-apps -u #2191 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-apps-pr/job/PR-2191/18/display/redirect

Test results:

  • PASSED: 1820
  • FAILED: 1

Those are the test failures:

org.kie.kogito.index.mongodb.ProcessDataIndexMongoDBKafkaIT.testProcessInstanceEvents 1 expectation failed.
JSON path data.UserTaskInstances[0].potentialGroups[0] doesn't match.
Expected: managers
Actual: null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow filtering based on ProcessIntance.definition field
3 participants