[DMS-912] Enable smoke test suites against local DMS stack#960
[DMS-912] Enable smoke test suites against local DMS stack#960simpat-jesus wants to merge 2 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| function Merge-DmsSpecs { |
simpat-jesus
left a comment
simpat-jesus
left a comment
There was a problem hiding this comment.
Inline notes anchoring each workaround to its specific upstream story (so each hunk becomes a git rm once the corresponding upstream change lands). Permanent improvements are also annotated where the structural intent is non-obvious from the code.
| } | ||
| } | ||
|
|
||
| function Merge-DmsSpecs { |
There was a problem hiding this comment.
Permanent improvement. The two-pass flow (one GenerateSdk per spec) used to let the descriptors pass clobber Client/HostConfiguration.cs written by the resources pass, leaving every resource-side IxxxApi DI registration absent (232 descriptor registrations only, vs the expected 396). Merging into one OpenAPI document and generating once eliminates the clobber as a structural class of bug; this is the correct shape independent of any upstream change.
| param ( | ||
| [string] | ||
| $ApiPackage, | ||
| function Rename-DescriptorOperationId { |
There was a problem hiding this comment.
Workaround. MetaEd.js emits operationIds like getGradingPeriods on both /ed-fi/gradingPeriods (resources-spec) and /ed-fi/gradingPeriodDescriptors (descriptors-spec). Once merged into a single spec, duplicate operationIds produce duplicate C# method names in Apis.All.
Removable when reference/design/backend-redesign/epics/08-relational-read-path/07-disambiguate-descriptor-operationids.md lands. Once descriptor operationIds carry the Descriptor/Descriptors suffix in MetaEd.js output, this helper plus the descriptor loop in Merge-DmsSpecs (lines 127-133) become dead code.
| # Drop required arrays on Homograph* schemas so the generichost-library generator omits its | ||
| # throw-on-missing-required validation. The smoke test tool's data factory cannot populate | ||
| # required props on these extension models, and the server-side spec still enforces them. | ||
| foreach ($schemaName in @($resources.components.schemas.Keys)) { |
There was a problem hiding this comment.
Workaround. OpenAPI Generator 7.19.0's generichost library validates required properties at deserialization, emitting throw new ArgumentException("Property is required for class HomographXxx", ...) blocks the smoke test tool's reflection-based data factory can't satisfy. Affected models: HomographContact, HomographSchool, HomographStaff, HomographStudent, HomographSchoolReference. Without this strip, the SDK throws at app setup before any test runs.
Removable when either (a) the generichost template adds an opt-out for the throw-on-missing-required validation, or (b) EdFi.LoadTools.SmokeTest updates its data factory to populate required constructor params on extension models. No upstream story yet tracks this; safe in the interim because the server-side spec still enforces the requirement (validation just shifts from client throw to server 400).
|
|
||
| # Download and parse OpenAPI spec | ||
| $spec = Invoke-WebRequest -Uri $Endpoint | ConvertFrom-Json | ||
| # Rewrite tags on non-ed-fi paths whose tag also appears on /ed-fi/* paths. Without this, |
There was a problem hiding this comment.
Workaround. MetaEd.js emits tag: contacts for both /ed-fi/contacts and /homograph/contacts even though it namespaces the URL path, operationId, and schema names. Without this rewrite, the generator emits a single ContactsApi class containing two Post methods, and the smoke test tool's categorizer fails with Multiple matching Post methods were found on type 'ContactsApi' before any test runs.
Removable when docs/stories/metaed-extension-openapi-tag-prefix.md lands and DMS picks up the new ApiSchema package. Once MetaEd.js emits tag: homograph_contacts directly, the coreTags collection and both foreach loops here (~20 lines) become dead code.
| endpoints.MapDelete(routePattern, DeleteById); | ||
| // /data/v3 alias for parity with ODS API URLs so SDK consumers (e.g. EdFi.LoadTools' | ||
| // SdkConfigurationFactory) that hardcode the historical /data/v3 base reach the same handlers. | ||
| string v3AliasPattern = routePattern.Replace("/data/{**dmsPath}", "/data/v3/{**dmsPath}"); |
There was a problem hiding this comment.
Workaround. EdFi.SmokeTest.Console 7.3.20144 (the net10-capable smoke test tool) hardcodes /data/v3 as the SDK BaseAddress suffix via EdFi.LoadTools's SdkConfigurationFactory, with no CLI/env/appsettings override. Without this parallel route registration every SDK call hits /data/v3/ed-fi/<resource> and DMS returns 400 (no such route).
Removable when docs/stories/ods-loadtools-data-path-configurable.md lands and DMS picks up a tool version that lets the smoke test wrapper pass --data-path /data (or equivalent). Once the tool stops hardcoding /data/v3, the alias variable plus the foreach (~10 lines) become dead code.
2 participants
Summary
Unblocks all three smoke suites end-to-end against a local DMS stack after the OpenAPI Generator 7.19.0 and smoke test tool upgrade. The two non-destructive suites now exit 0;
DestructiveSdkruns to completion (no categorizer / token-exchange / SDK-generation blockers) and exits 1 only on a pre-existing tool ↔ DMS mismatch documented below.NonDestructiveApiNonDestructiveSdkDestructiveSdkPrior state was exit 1 across the board:
Multiple matching Post methods were found on type 'ContactsApi'blocking the SDK suites at categorization time, and a 502 Bad Gateway blocking the API suite at token-exchange time.Changes
Permanent improvements
build-sdk.ps1— single-pass merged-spec generation. NewMerge-DmsSpecsunions the resources and descriptors specs into one OpenAPI document so the generator runs once and emits a completeHostConfiguration.cs/IApi.cs. The prior two-pass flow let the descriptors pass clobber the resources-passHostConfiguration, leaving every resource-sideIxxxApiDI registration absent (232 descriptor registrations only). With the merge in place, all 396 registrations (resources + descriptors) survive into the SDK.Get-OperationIdsandBuild-CollisionMappingsdeleted as dead code.eng/smoke_test/Invoke-*Tests.ps1— pin all three wrappers toEdFi.SmokeTest.Console 7.3.20144 -PreRelease. Future tool bumps touch one number.eng/smoke_test/modules/SmokeTest.psm1—Get-SmokeTestConsolePathdiscovers the latestnetX.Yframework directory undertools/instead of hardcodingnet8.0. Adds an explicit-OAuthUrloption so callers can route the token endpoint independently ofBaseUrl.eng/Package-Management.psm1—Get-NugetPackageshort-circuits when the requestedlowerId.versiondirectory is already cached under.packages/, avoiding a redundant remote round-trip.Temporary workarounds (each ties to a specific upstream story; once that lands, the corresponding hunk becomes dead code)
build-sdk.ps1extension tag rewrite (coreTags+ tag-rewrite loops inGenerateSdk)tag: contactsfor both/ed-fi/contactsand/homograph/contacts. Without the rewrite, the generator emits a singleContactsApiwith twoPostmethods and the smoke test tool's categorizer fails.metaed-extension-openapi-tag-prefixlands and DMS picks up the new ApiSchema package.build-sdk.ps1descriptor operationId rename (Rename-DescriptorOperationId+ descriptor loop inMerge-DmsSpecs)getGradingPeriodson both/ed-fi/gradingPeriodsand/ed-fi/gradingPeriodDescriptors); once merged, duplicate operationIds produce duplicate C# method names.disambiguate-descriptor-operationidslands and descriptor operationIds carry theDescriptor/Descriptorssuffix in MetaEd.js output.build-sdk.ps1Homographrequiredstrip (end ofMerge-DmsSpecs)generichostlibrary throwsArgumentExceptionat deserialization when required reference types are absent; the smoke test tool's reflection-based data factory does not populate constructor-required reference types on the five affectedHomograph*models.generichosttemplate adds an opt-out for throw-on-missing-required validation, orEdFi.LoadTools.SmokeTestupdates its data factory for extension models.src/dms/.../CoreEndpointModule.cs/data/v3parallel routeEdFi.SmokeTest.Console 7.3.20144hardcodes/data/v3as the SDKBaseAddresssuffix with no CLI/env/appsettings override. Without the alias, every SDK call hits/data/v3/ed-fi/<resource>and DMS returns 400.ods-loadtools-data-path-configurablelands and DMS picks up a tool version that lets the smoke test wrapper override--data-path.Inline review comments on this PR call out each workaround and its specific upstream story.
DestructiveSdkResidual Errors — Out of Scope, Tracked UpstreamThe four
DestructiveSdkerrors are not addressed in this PR. Root cause (verified 2026-05-13 by decompilingEdFi.LoadTools.dlland correlating its log with DMS container traces and direct curl reproduction):EdFi.LoadTools.SmokeTest.PropertyBuilders.BaseBuilder.BuildRandomNumberstarts a counter at 1000 and increments per call as the default for requiredint/long/doubleproperties when the OpenAPI spec it fetches at runtime carries nominimum/maximum. The DMS-published OpenAPI spec emits required numeric fields as plaintype: number, format: doublewith no bounds (the per-field precision rules live indecimalPropertyValidationInfoson eachresourceSchemasentry, not in the OpenAPI fragment). OnSample.BusRoute.hoursPerWeekandSample.StudentGraduationPlanAssociation.hoursPerWeek— both requireddoubleproperties withtotalDigits=5, decimalPlaces=2(cap ±999.99) — the factory's ≥ 1000 default trips DMS'sValidateDecimalMiddleware, producing 400 responses with body{"$.hoursPerWeek":["hoursPerWeek must be between -999.99 and 999.99."]}(320 bytes, matching the smoke test log'sRaw content length: 320byte-for-byte). The twoGetAllTestfailures cascade — no row created → empty 200 → "expected at least one".This is a pre-existing tool ↔ server mismatch, not a regression introduced by DMS-912's enablement work. The mismatch only became observable on this branch because the categorizer fix (item 1 in the workaround table above) lets the destructive suite reach individual
PostTests for the first time. Manual POSTs against the same DMS endpoints withhoursPerWeek ≤ 999.99succeed (201 Created), and POSTs withhoursPerWeek ≥ 1000reproduce the exact 400 the tool sees — confirming the factory's default range is the sole rejection criterion.Fix tracked as a separate upstream story against
Ed-Fi-Alliance-OSS/Ed-Fi-ODS:docs/stories/ods-loadtools-decimal-validation-aware-defaults.md(uncommitted; sibling to the other upstream-story drafts indocs/stories/). The story asks LoadTools to use a numeric fallback that stays within commonly-tight Ed-Fi decimal caps when the served spec under-specifies bounds, and notes the complementary DMS-side option (projectdecimalPropertyValidationInfosonto the published spec) as the server-side counterpart — landing either or both removes these four errors.Test plan
pwsh start-local-dms.ps1 -EnableConfig -EnableSwaggerUI -LoadSeedData -AddSmokeTestCredentials -IdentityProvider self-contained -SearchEngine OpenSearchpwsh ./build-sdk.ps1 BuildAndGenerateSdk -PackageName EdFi.DmsApi.TestSdk -DmsUrl http://localhost:8080(single merged invocation;HostConfiguration.cscontains both resources and descriptors registrations)pwsh ./Invoke-NonDestructiveApiTests.ps1 -BaseUrl http://localhost:8080 -Key $KEY -Secret $SECRET->Exit Code is 0pwsh ./Invoke-NonDestructiveSdkTests.ps1 -BaseUrl http://localhost:8080 -Key $KEY -Secret $SECRET -SdkPath <TestSdk.dll>->Exit Code is 0pwsh ./Invoke-DestructiveSdkTests.ps1 -BaseUrl http://localhost:8080 -Key $KEY -Secret $SECRET -SdkPath <TestSdk.dll>-> runs to completion; expected exactly 4 errors onSample.BusRoute+Sample.StudentGraduationPlanAssociationhoursPerWeekcap, tracked byods-loadtools-decimal-validation-aware-defaults