Fix groups api calls - #61
Draft
edeati wants to merge 82 commits into
Draft
Conversation
… the RF2 content is loaded into the DB, and BEFORE the tests run
edeati
marked this pull request as draft
April 28, 2023 04:07
CoderMChu
requested changes
May 17, 2023
CoderMChu
left a comment
CoderMChu
left a comment
Member
There was a problem hiding this comment.
@edeati Thanks for your pull request. Well spotted and thanks for fixing the group api call. This api is not used internally and should have been removed. It would be great if you can split your changes in 3 different PRs. (i.e group api call fix, docker-compose change and azure pipeline)
| - 8081:8081 | ||
| environment: | ||
| - SPRING_DATASOURCE_URL=jdbc:mysql://db:3306/?useSSL=false | ||
| - SPRING_DATASOURCE_URL=jdbc:mysql://db:3306/?useSSL=false&llowLoadLocalInfile=true&allowPublicKeyRetrieval=true |
Member
There was a problem hiding this comment.
"llowLoadLocalInfile=true" is missing a (i.e allowLoadLocalInfile=true)
| - mysql:/var/lib/mysql | ||
| command: | ||
| mysqld --sql_mode="NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES" --lower_case_table_names=1 | ||
| mysqld --local-infile=ON --sql_mode="NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES" --lower_case_table_names=1 |
QuyenLy87
force-pushed
the
develop
branch
2 times, most recently
from
August 21, 2025 10:21
9297e60 to
d00441a
Compare
QuyenLy87
force-pushed
the
develop
branch
4 times, most recently
from
September 9, 2025 07:56
cb2fb0f to
ac59775
Compare
term was VARCHAR(333), which silently truncates AMT terms - the longest in the AU daily build is 2,027 characters - and fabricates roughly 9,700 false failures per run. It is also why fix-long-terms.sh exists at all: RVF rewrites every term >= 255 characters to first-100-chars + MD5 before loading, purely so the values fit. 333 is not arbitrary. MyISAM caps a key at 1000 bytes and utf8 is 3 bytes per character, so 333 * 3 = 999 is the widest indexable utf8 column. This fork added `key idx_term(term)`, which pinned the column to that width. An earlier attempt to widen it (24b6658) was reverted (9643b9a) because the index then fails outright: varchar(4096) + key idx_term(term) -> ERROR 1071: Specified key was too long; max key length is 1000 bytes varchar(4096) + key idx_term(term(333)) -> works, Sub_part = 333 A PREFIX index keeps the index while letting the column hold a full-length term. Selectivity is unchanged: today only the first 333 characters exist at all, so indexing exactly those characters indexes everything the current schema can hold. Note IHTSDO master already uses varchar(4096) throughout and does NOT index term, so catching this fork up would resolve the same problem by dropping the index. This change keeps it. Verified: all 90 tables create on MySQL 8.0.34/MyISAM, term columns report character_maximum_length 4096, and both term indexes report sub_part 333. Impact: assertions will see full-length terms for the first time. The ~9,700 false failures per run should disappear, and genuine findings that truncation was hiding - 8 descriptions exceeding the length the release itself declares - will surface. fix-long-terms.sh becomes unnecessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Store full-length terms: VARCHAR(4096) with a prefix index
Both clones were unpinned, so the image was not reproducible: what you got depended on when it was built, and an image that works could stop working with no change to this repository. That is what happened today. This image had not been rebuilt since 2024-05-24. Merging PR #12 triggered the first rebuild in two years, which picked up two years of upstream drift, and RVF failed to start: FileNotFoundException: .../release-type-delta-previous-snapshot-validation- inferred-relationship_EDITION.sql The RVF server never came up, the k8s job was terminated after ~12 minutes, and daily-rvf failed with no report produced. The skew is between two repositories, which is why neither looked broken on its own: manifest.xml comes from the helm chart and references four _EDITION scripts, while the SQL scripts are cloned from IHTSDO at image build time and IHTSDO removed those variants in 9cea111 (2024-08-19). Our own code never mentions _EDITION - the filename comes from the manifest's sqlFile attribute at runtime. Assertions pinned to fad36466 (2024-05-23), the repository state the last known-good image (build 4556) was built from. Verified: all four _EDITION scripts the mounted manifest references resolve at that commit. Drools rules pinned to 55795d5d. This does NOT fix Drools - those rules need snomed-drools 5.7.0 and this branch pins 4.0.0, so rule compilation keeps failing silently exactly as it does today. It makes that state reproducible instead of dependent on build date. PR #13 supplies the 5.7.0 bump; the two become a matched pair once it lands. Both pins are ARGs so they can be overridden without editing the file, and the comment records what to check when bumping either one. This restores the working state. It does not resolve the underlying divergence - our manifest expects filenames upstream abandoned two years ago. That reconciliation belongs with the IHTSDO catch-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pin the Dockerfile's two git clones
Drools validation has been silently contributing nothing to RVF runs. The rules loaded at runtime from IHTSDO/snomed-drools-rules call service methods that do not exist in snomed-drools 4.0.0, so rule compilation fails, no Drools results are produced, and the build still reports success. snomed-parent-bom 2.0.0 pins <snomed-drools.version>4.0.0</snomed-drools.version> and its dependencyManagement resolves the engine and rf2-validator artifacts through that property, so overriding the property in this pom is enough -- no dependency declarations change. 5.7.0 moved validateRF2Files to a 7-arg signature, inserting an assertionExclusionList parameter in 4th position. We pass null for it and for the validator's module filter, because this fork does its own module filtering after the call using common-authoring group membership. Verified: - dependency:tree resolves snomed-drools-engine and snomed-drools-rf2-validator at 5.7.0 - mvn compile succeeds (118 sources) - all 17 service methods called across the 109 .drl files in snomed-drools-rules@develop are present in 5.7.0's ConceptService, DescriptionService and RelationshipService interfaces (0 missing) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The version bump alone does not fix Drools. Running the new parity harness
against a real release reproduced the rule-compilation failure:
Unable to build KieBaseModel:defaultKieBase
unable to resolve method: org.kie.api.definition.type.Annotation.active()
could not resolve constructor InvalidContent[String, Annotation, String, Severity]
The blocker is org.ihtsdo.drools.domain.Annotation, which exists at 6.0.0 and
not at 5.7.0. The rules began using it in b64999b (2026-04-09). With 5.7.0 on
the classpath DRL silently resolves Annotation to the unrelated
org.kie.api.definition.type.Annotation, which is why the errors name methods
nobody wrote. The isFirstCharUppercase and
findMissingPreferredTermPerLanguageRefset errors in the same output are
knock-on; both are DRL functions defined in the rules themselves.
6.0.0 is not an option here: it is compiled for Java 25 (class file version
69) and this image runs 17. That needs upstream's PIP-1048
amazoncorretto:17->25 bump, i.e. the IHTSDO catch-up, not a version bump.
So DROOLS_RULES_REF moves to 45e0d9e2 (2026-04-03), the last commit before
Annotation. Verified: rules compile against 5.7.0 with zero errors on Java 17.
The consequence is that Drools runs four months behind current develop, which
is a deliberate trade and is recorded in the Dockerfile.
Why the earlier check missed this: it extracted every *Service.method( call
from the .drl files and confirmed all 17 exist in 5.7.0. That was true, and
insufficient - it could not see a missing domain TYPE. Closing exactly that
kind of gap is what the harness is for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fix Drools: snomed-drools 5.7.0 + rules pinned to a compatible commit
dockerfile-maven-plugin was configured with a repository but no tag, so it
defaulted to `latest` - the exact tag values.yaml pins the Helm chart to. Every
build therefore replaced production's image, including one-off builds queued
against experimental branches.
That has already happened. The last two pushes to `latest` were:
rvf-20260810.2 refs/heads/drools-validate-delta 2026-08-10
rvf-20260810.1 refs/heads/fix-aws-sdk-runtime 2026-08-10
drools-validate-delta is documented as abandoned and wrong by construction - it
hands Drools the prospective's own snapshot and its own delta, manufacturing
6,622 false duplicate-relationship failures. It is inert today only because the
rules do not compile, so Drools never runs. Anyone fixing the Drools dependency
without also rebuilding the image would have found those failures waiting.
The production branch keeps publishing `latest`; every other branch publishes
under its own name. That is also what the :catchup image needs, so the same
change unblocks standing up the parallel pipeline.
The pom default stays `latest` deliberately: it only applies to a bare
`mvn dockerfile:push`, and changing it would alter behaviour for a path that
was never the problem. The protection belongs in the pipeline, which is where
the clobbering actually came from.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018aYBXcyiozxQQGhzzhS7KG
Tag images per branch so only groups-api-fix publishes :latest
PR #13 got Drools past the version wall and straight into the next one. Build 15836, on the Drools-fixed image: System Failure: java.lang.NoClassDefFoundError: software/amazon/awssdk/auth/credentials/AwsCredentialsProvider at ValidationRunner.doRunValidations(ValidationRunner.java:167) This is thrown while ValidationRunner sets Drools up, so it fails the ENTIRE validation: TestResult came back null, no report was produced, and rvf2junit then died on the null. That is strictly WORSE than the bug PR #13 fixed - before, Drools failed at rule compilation, was caught, contributed nothing, and validation completed. One silent defect became a total run failure. :latest has been rolled back to known-good-20260807-pinned so tonight's scheduled run is unaffected. Cause: snomed-drools 5.7.0 is compiled against resource-manager 7.7.0, whose ResourceManager and RuleExecutor reference software.amazon.awssdk types. snomed-parent-bom 2.0.0 manages otf-common - and with it resource-manager - DOWN to 6.0.0, which is the older spring-cloud-aws-core line carrying only AWS SDK v1. dependency:tree -Dverbose says it plainly: resource-manager:jar:6.0.0 - version managed from 7.7.0; omitted for duplicate Before the 5.7.0 bump Drools never reached that code, so the gap was invisible. Letting otf-common resolve forward to 7.7.0 was tried first, as the fix that addresses the cause. It does not stop there: the 7.7.0 line is Jakarta and SDK v2 throughout, and pulls javax.jms -> jakarta.jms plus AmazonS3 -> S3Client migrations into four files. That is a slice of the modernisation the IHTSDO catch-up performs wholesale, and doing it twice - once here on Java 17 and again there - is waste. So the SDK is declared explicitly instead, at the version the 7.7.0 chain resolves. Verified: 78 tests pass, and AwsCredentialsProvider, S3Client and ResourceManager all load from the built runtime classpath. The same combination - resource-manager 6.0.0 with the SDK v2 jars present - is what the Drools parity harness has been running against locally, where DroolsRF2Validator constructs, rules compile and 3.6M service-layer checks agree. Still unproven until a real run: that the validation completes end to end. This round is the reminder that compiling, and even rules compiling, does not establish that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add AWS SDK v2, required at runtime by snomed-drools 5.7.0
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
6 participants
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.
This is a fix for
groupsapi calls where the api call throws an exception:com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class org.hibernate.proxy.pojo.bytebuddy.ByteBuddyInterceptor and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) (through reference chain: org.ihtsdo.rvf.core.data.model.AssertionGroup$HibernateProxy$PhV4dVWY["hibernateLazyInitializer"])To reproduce this either try to get an existing assertion group with a
GET groups/{id}call or try to add new tests to an assertion group with aPOST groups/{id}/assertions