Repository navigation
fix: cross-field comparison rule across ports and C# concrete-parent properties - #531
Merged
Merged
Conversation
…ies; fix test timing flakes A C# entity that extends another skipped the inherited fields but its class declaration never named the base, so the parent's properties appeared nowhere. A concrete parent is flattened onto the child (each concrete entity owns a table with all its columns; class inheritance between mapped entities would be an EF TPH/TPT hierarchy). An emitted abstract shape is named as the base. Flakes fixed at the cause: DependencyCliTests changed the process-wide cwd outside the serial collection, racing GenCommandTests; codegen-ts compile tests run under a 30s timeout set in one helper instead of bun's 5s default.
…executed Patch check; TPH form rule test A validator.comparison between fields of different kinds (e.g. date vs timestamp) is now skipped in Java, Kotlin, TypeScript and Python as C# already did, instead of a 500 (ClassCastException), a string compare, or a TypeError. Adds the shared comparison-across-kinds-is-skipped conformance case, replaces the Java Patch source-text comparison with an executed check, and adds a TPH subtype form test for a cross-field rule.
…ays enforced; review fixes Code review of the cross-kind skip found that equality on identifier-like fields was dropped along with ordering. Restores it in every port, adds shared uuid cases, corrects stale C# comments, and documents the EmitClassDeclarationLine seam change.
…r rejects across kinds
…kip cross-field comparison
…nd-based comparison rule; one follow-up left.
…Mapper An ejected copy of KotlinCrossFieldRules compiles standalone, so it cannot reference KotlinTypeMapper. Decide the native shape from @localTime and @lenient directly.
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
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.
What Changed
class Child : Baseand declares only its own members. Removed the dead special-case guard in the cross-fieldValidateoverride. AddedEntityExtendsParentTests.validator.comparisonnow follows one rule in all five ports. Numerics compare with each other. Strings and temporals compare only with the same subtype (temporals also need matching@localTime). Boolean, enum, uuid, uri and inet fields test only eq/ne. Any other pair is skipped, never rejected. Java/Kotlin no longer cast toComparable(create returned 500 instead of 400), TypeScript no longer compares as strings, and Python no longer raisesTypeError. Ordering on boolean and enum is no longer enforced by generated validators. Added shared conformance cases infixtures/validation-conformance/, documented in its README, and re-synced the owned zod-validators template copies.DependencyCliTestsnow joins the serialOwnedRunnerCollection, fixing a working-directory race. The codegen-ts compile tests share a 30s timeout fromtest/compile-timeout.ts. The Java Patch cross-field test executes the generated Dto and Patch classes instead of comparing source text. A TS React regression test covers a TPH subtype form binding the refined body schema. Adds a test-scopejackson-datatype-jsr310tocodegen-springfor the conformance runner.Flaky tests fixed (at the cause, no retries)
GenCommandTests.Gen_writes_the_hash_manifest_beside_the_metadata_not_in_cwd:DependencyCliTestsrunsgenin process, which changes the process-wide working directory, without joining the serialOwnedRunnerCollection; the two classes ran in parallel and one read the other's directory as the cwd (about one run in five). Now serial; 12 of 12 looped runs passed after the fix.tph-cross-field.test.ts): bun's 5s default timeout against a slowest measured 3.3s case under load. Each file now callssetDefaultTimeout(COMPILE_TEST_TIMEOUT_MS)(30s, defined once intest/compile-timeout.ts; a side-effect import would only reach the first file per process). The original timeouts were not reproduced on demand, so this one is justified by measurement.Gate runs
Two no-mistakes runs for this PR. Run 1 failed at the test step before any push because GitHub SSH auth was down during a GitHub incident (infrastructure, not code); its review fixes were carried into run 2, which produced this PR. The test step of run 2 was approved past an "inconclusive" live-evidence verdict after the maintainer-requested serial local suites all passed (C# Codegen 1128, Cli 364, Conformance 2222; Java/Kotlin cross-field and conformance; Python 1304; TS cross-field conformance 25, codegen-ts 71, codegen-ts-react 62).
Validation
Risk Assessment
✅ Low: Codegen and test-only changes, bounded and consistent across all five ports; the prior-round fixes (@localTime comparability, C# eq/ne CLR-type guard, per-file timeouts) check out against the code, and I found no regression I could substantiate.
Testing
Live-driven: per-file compile timeout probe on the real bun runner (pass). Suite-only, not live: TS validation conformance (85 pass), C# codegen (1128 pass, incl. EqAcrossClrTypes), C# conformance (2222 pass), C# CLI (364 pass). Not driven: Java, Kotlin and Python conformance and unit suites, java-reactor build, integration-tests lanes, full ci-local.sh run. Verdict inconclusive: live-exercisable surface exists but only one scenario was driven live.
Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
server/typescript/packages/codegen-ts/test/tph-cross-field.test.ts:164- The intent says every codegen-ts test that type-checks generated output with the real TypeScript compiler runs under the shared 30s timeout. tph-cross-field.test.ts does this, but it is not one of the 28 files. It callsts.createProgramat line 123, and its test "the generated entity, queries and routes compile" (line 164) runs that compile. The file never callssetDefaultTimeout(COMPILE_TEST_TIMEOUT_MS). Bun scopes that call to the file that makes it, so this test keeps bun's 5s default. On a loaded machine it can still report the timeout flake this change says it fixes. CI passes--timeout 30000, so CI does not hit it. A plainbun testin the package can. Fix: importsetDefaultTimeoutandCOMPILE_TEST_TIMEOUT_MSfrom./compile-timeout.js, and call it at the file's top level, as the other 28 files do. The file came from feat: enforce cross-field validators in every port's generated create models #530 and is not in this diff. The gap is in the coverage this change claims. Every othercreateProgramuser under test/ already makes the call.🔧 Fix applied.
1 warning still open:
server/csharp/MetaObjects.Codegen/Generators/EntityGenerator.cs:1159- The eq/ne fallback in the C# comparison emitter requires only the same field subtype, not the same CLR type.loggedAt eq wallClock(field.timestampvsfield.timestamp @localTime) misses the temporal branch because ScalarForField differs (DateTimeOffset vs DateTime). It then reaches!ordering && l.SubType == r.SubType && lk != Otherand emitsEquals(__l, __r). That compares a boxed DateTimeOffset with a boxed DateTime, which is always false, soeqrejects every body where both fields are set.nenever fires. Java, Kotlin, TypeScript and Python skip this pair, and the intent says an unsound pair is 'skipped, never rejected'. The new conformance case only exercisesltefor this pair, so it does not catch it. The same hole exists for two enum fields with different generated enum types (EnumPropertyTypeName differs, so Equals is always false) and for a@lenienturi/inet (string) against a strict uri/inet (Uri/IPAddress). The latest fix round (a25bd2b) added the@localTimecheck to the temporal ordering branch only and left this equality fallback behind. Fix: in the fallback also requireCSharpNaming.ScalarForField(l) == CSharpNaming.ScalarForField(r), and for enums require equal EnumPropertyTypeName. Add aneqconformance variant for the instant vs wall-clock pair.🔧 Fix applied.
✅ Re-checked - no issues remain.
server/java/codegen-spring/src/main/java/com/metaobjects/generator/spring/SpringGeneratorSupport.java:679- Java/Kotlin/Python ports' soundness fixes lack live evidence in this run.scripts/ci-local.sh --only ts-fast --only ts-unit --strict-toolchainscd server/typescript/packages/integration-tests && bun test test/validation-conformance.test.ts test/validation-conformance-runtime.test.ts (85 pass, 0 fail; suite run, not live)Probe in server/typescript/packages/codegen-ts: two temporary test files, one calling setDefaultTimeout(COMPILE_TEST_TIMEOUT_MS) and one not, 6s test each, run via plain bun test in both file orders. Calling file passes at 30s; non-calling file fails at 5000ms as designed. Probes deleted afterward.dotnet test server/csharp/MetaObjects.Codegen.Tests (1128 pass, 0 fail, 1 skipped; suite run, not live)dotnet test server/csharp/MetaObjects.Conformance.Tests (2222 pass, 0 fail; suite run, not live)dotnet test server/csharp/MetaObjects.Cli.Tests (364 pass, 0 fail; suite run, single run, not live)Inspected codegen-ts test/ for setDefaultTimeout coverage: 30 files call setDefaultTimeout; no createProgram user without itBaseline ts-fast/ts-unit from the no-mistakes run, not repeated🔧 **Document** - 1 issue found → auto-fixed ✅
agent-context/skills/metaobjects-authoring/SKILL.md:437- Authoring skill comparison row omits the kind rule (only numerics, own-subtype strings/temporals, eq/ne on boolean/enum/uuid/uri/inet; other pairs skipped). The file is mirrored into six fixtures/agent-context-conformance/*/expected copies, so editing it needs those regenerated. Also spec/metamodel/validator.json rules text is schema-backed and generated into fixtures/metamodel-docs; it is not wrong, so left alone.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.