UNOMI-920: Improve debugging with YAML-based toString() for core API types#768
Open
sergehuber wants to merge 25 commits into
Open
UNOMI-920: Improve debugging with YAML-based toString() for core API types#768sergehuber wants to merge 25 commits into
sergehuber wants to merge 25 commits into
Conversation
- Add YamlUtils (SnakeYaml) with YamlConvertible, YamlMapBuilder, circular reference detection (identity-based visited sets), and recursion depth limits. - Implement YAML-backed toString()/toYaml() on core API types extending Item / MetadataItem (Condition, ConditionType, Action, ActionType, Rule, Segment, Goal, Scoring, ScoringElement, Parameter, Metadata, etc.). - Add YamlUtilsTest coverage for builder, toYamlValue, and representative rules. Build and integration alignment: - unomi-api: snakeyaml + test dependencies; manage mockito-core version in BOM. - RESTParameter: use Object for defaultValue to match Parameter#getDefaultValue(). - RulesServiceImpl: avoid NPE when tracked parameter defaultValue is null before split. - itests: override awaitility to 3.1.6 for OSGi (Hamcrest 1.3 bundle); Karaf itests common logback exclusions; hamcrest bundle scope test.
Run mikepenz/action-junit-report after every integration matrix leg (if: always), with update_check and per-matrix check_name, so a green re-run replaces the stale red JUnit check. Use fail_on_failure=false and require_tests=false; continue-on-error so missing XML cannot fail the job.
Use identity-based visited sets for circular-reference detection, harden Condition deepCopy and parameter handling, fix Parameter max-depth YAML, wire slf4j.version in unomi-api, and expand unit test coverage.
Replace the com.github.alexcojocaru:elasticsearch-maven-plugin (binary download + forked JVM) with io.fabric8:docker-maven-plugin so the elasticsearch profile of itests runs ES in a Docker container, mirroring how the opensearch profile already runs OpenSearch. itests/pom.xml (elasticsearch profile) * Add an <elasticsearch.port>9400</elasticsearch.port> property and pass it through the failsafe systemPropertyVariables so tests resolve the HTTP port from a single source. * Replace the elasticsearch-maven-plugin block with a docker-maven-plugin block that starts/stops a docker.elastic.co/elasticsearch/elasticsearch container, binds target/snapshots_repository to /tmp/snapshots_repository, and waits on the HTTP port before the integration-test phase. * Add a chmod -R ugo+rwx on snapshots_repository in the antrun unzip step: the ES container runs as UID 1000, so on Linux CI the bind-mounted snapshot repo otherwise hits access_denied during repository verify. pom.xml (root) * Declare <docker-maven-plugin.version>0.48.0</docker-maven-plugin.version> and add the pluginManagement entry so the elasticsearch profile (and any future user of the plugin) inherits a single version. This is phase A0 of the PR #757 stack split (see docs/PR-757-stack-extraction-tracker.md). It is intentionally small and self-contained: no test/code changes, only the test infrastructure switch.
…icsearch in integration tests Implements the core configuration switch from UNOMI-921: https://issues.apache.org/jira/browse/UNOMI-921 Replace the com.github.alexcojocaru:elasticsearch-maven-plugin (binary download + forked JVM) with io.fabric8:docker-maven-plugin in the elasticsearch profile of itests, mirroring how the opensearch profile already runs OpenSearch in a Docker container. itests/pom.xml (elasticsearch profile) * Add an <elasticsearch.port>9400</elasticsearch.port> property and pass it through the failsafe systemPropertyVariables so tests resolve the HTTP port from a single source (unchanged from the previous 9400). * Replace the elasticsearch-maven-plugin block with a docker-maven-plugin block that runs docker.elastic.co/elasticsearch/elasticsearch:${elasticsearch.test.version}, binds target/snapshots_repository to /tmp/snapshots_repository, and waits on the HTTP port before the integration-test phase. Heap aligned to 8GB (-Xms8g -Xmx8g) to match the OpenSearch configuration and the ES 9 recommendation. Discovery=single-node, replicas=0, xpack.ml and xpack.security disabled. * Container lifecycle matches OpenSearch exactly: pre-integration-test runs stop+remove then start (with showLogs); post-integration-test runs stop only -- container is kept around for inspection. * Add a chmod -R ugo+rwx on snapshots_repository in the antrun unzip step: the ES container runs as UID 1000, so on Linux CI the bind-mounted snapshot repo otherwise hits access_denied during repository verify. pom.xml (root) * Declare <docker-maven-plugin.version>0.48.0</docker-maven-plugin.version> and add the pluginManagement entry so the elasticsearch profile (and any future user of the plugin) inherits a single version. Scope kept minimal for the PR #757 stack split: only the test infrastructure switch lives here. The follow-up UNOMI-921 acceptance items below ship in the platform PR (P) once it lands: * Remove BaseIT.fixDefaultTemplateIfNeeded() and the call in checkSearchEngine() (no longer needed with Docker). * Migrate16xToCurrentVersionIT: replace hardcoded ES_BASE_URL = "http://localhost:9400" with dynamic getSearchPort(). * Drop the comments referring to the elasticsearch-maven-plugin template-override workaround. See docs/PR-757-stack-extraction-tracker.md for the full split plan and how this PR fits in the stack.
Non-interactive prompts when CI, GITHUB_ACTIONS, or BUILD_NON_INTERACTIVE is set. Add --ci (no Karaf, no Maven build cache) and MAVEN_EXTRA_OPTS for matrix ports.
Replace fixed sleeps in GraphQLListIT and poll after patch refresh in PatchIT.
Poll profile properties after events in PropertiesUpdateActionIT.
Align CI with local developer workflow; pass matrix ports via MAVEN_EXTRA_OPTS.
…icsearch in integration tests Implements the core configuration switch from UNOMI-921: https://issues.apache.org/jira/browse/UNOMI-921 Replace the com.github.alexcojocaru:elasticsearch-maven-plugin (binary download + forked JVM) with io.fabric8:docker-maven-plugin in the elasticsearch profile of itests, mirroring how the opensearch profile already runs OpenSearch in a Docker container. itests/pom.xml (elasticsearch profile) * Add an <elasticsearch.port>9400</elasticsearch.port> property and pass it through the failsafe systemPropertyVariables so tests resolve the HTTP port from a single source (unchanged from the previous 9400). * Replace the elasticsearch-maven-plugin block with a docker-maven-plugin block that runs docker.elastic.co/elasticsearch/elasticsearch:${elasticsearch.test.version}, binds target/snapshots_repository to /tmp/snapshots_repository, and waits on the HTTP port before the integration-test phase. Heap aligned to 8GB (-Xms8g -Xmx8g) to match the OpenSearch configuration and the ES 9 recommendation. Discovery=single-node, replicas=0, xpack.ml and xpack.security disabled. * Container lifecycle matches OpenSearch exactly: pre-integration-test runs stop+remove then start (with showLogs); post-integration-test runs stop only -- container is kept around for inspection. * Add a chmod -R ugo+rwx on snapshots_repository in the antrun unzip step: the ES container runs as UID 1000, so on Linux CI the bind-mounted snapshot repo otherwise hits access_denied during repository verify. pom.xml (root) * Declare <docker-maven-plugin.version>0.48.0</docker-maven-plugin.version> and add the pluginManagement entry so the elasticsearch profile (and any future user of the plugin) inherits a single version. Scope kept minimal for the PR #757 stack split: only the test infrastructure switch lives here. The follow-up UNOMI-921 acceptance items below ship in the platform PR (P) once it lands: * Remove BaseIT.fixDefaultTemplateIfNeeded() and the call in checkSearchEngine() (no longer needed with Docker). * Migrate16xToCurrentVersionIT: replace hardcoded ES_BASE_URL = "http://localhost:9400" with dynamic getSearchPort(). * Drop the comments referring to the elasticsearch-maven-plugin template-override workaround. See docs/PR-757-stack-extraction-tracker.md for the full split plan and how this PR fits in the stack.
…MI-921-itests-es-docker
# Conflicts: # build.sh # itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java # itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java
# Conflicts: # build.sh # itests/pom.xml # itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
…and deepCopy collection type
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
JIRA: https://issues.apache.org/jira/browse/UNOMI-920
Why this change
Core Unomi API objects are deeply nested and interconnected — conditions recurse, rules bundle conditions and actions, metadata types carry parameters, and real graphs can contain cycles (for example when inspecting profile ↔ event ↔ session). In many places,
toString()is missing, falls back toObject.toString(), or uses ad-hoc concatenation that is hard to read in logs and breakpoints. That slows down live debugging, operational triage, and support, because engineers cannot see structure and field names at a glance.YAML-formatted output gives indentation, named fields, and readable nesting, which makes logged or inspected values far easier to scan than opaque object identities or flat string blobs. Together with explicit circular-reference markers, we get safe, repeatable dumps suitable for logs and copy/paste into issues or docs.
What changed
YamlUtils(SnakeYaml) withYamlConvertible,YamlMapBuilder, identity-based cycle detection, optional depth limits, and$ref: circularwhere the same instance reappears.toYaml/toStringon keyItem/MetadataItemtypes (includingCondition,ConditionType,Action,ActionType,Rule,Segment,Goal,Scoring,ScoringElement,Parameter,Metadata, etc.).YamlUtilsTestfor the helper API and representative usage.Supporting changes
unomi-api: SnakeYAML + test dependencies;mockito-coreversion managed inunomi-bom(mockito.version).RESTParameter:defaultValueasObjectto matchParameter#getDefaultValue().RulesServiceImpl: avoidnulldefaultValuebefore splitting tracked-condition parameter strings.unomi-itests: pinawaitilityto 3.1.6 for Karaf/OSGi (Hamcrest 1.3 bundle), Karaf itests logback exclusions, Hamcrest bundletestscope (aligned withunomi-3-dev).Review history (from #754)
All 8 Copilot review comments were addressed in commit
bcf5a6898:YamlUtils.javaHashSet(equality-based), risking false "already visited" hits on types that overrideequalsby contentYamlUtils.newIdentityVisitedSet()(IdentityHashMap-backed); all implementations switched to it; javadoc updated; test addedCondition.javadeepCopy()had no cycle guard — self-referential graphs would causeStackOverflowErrordeepCopy()now uses anIdentityHashMapvisited set and throwsIllegalStateExceptionwith a clear message on cycle detectionCondition.javagetParameter()impliedparameterValuescan be null, butcontainsParameter/setParameter/equals/hashCodewould NPEsetParameterValues(null)normalised to emptyHashMap;containsParameter/setParametermade null-safe; tests addedParameter.java"validation"key — copy/paste artifact producing misleading YAMLid,type,multivalued,defaultValue) with"<max depth exceeded>"placeholderParameter.javaserialVersionUIDchanged — could break cross-version Java deserializationItem.javaserialVersionUIDconcern for the widely-usedItembase classpom.xml<slf4j.version>property that was not referenced anywhereslf4j-apiandslf4j-simpleinunomi-api/pom.xmlwith a clarifying commentYamlUtilsTest.javaHashMapinstances