phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

StateObject validation is not exclusive of extra keys #290

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

There has been a regression since some StateSchema work in which validation that keeps the stateObject looking like the stateSchema is not working correctly. This was originally identified in https://github.com/phetsims/ph-scale-basics/issues/56#issuecomment-1400799512.

zepumph commented 1 year ago

Patch to reproduce a silent failure of validation:

```diff Subject: [PATCH] reproduuuuce --- Index: tandem/js/types/IOType.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/IOType.ts b/tandem/js/types/IOType.ts --- a/tandem/js/types/IOType.ts (revision 3976db7a368cca0d06f02c9146145c76894d369a) +++ b/tandem/js/types/IOType.ts (date 1674683808300) @@ -441,6 +441,9 @@ * Assert if the provided stateObject is not valid to this IOType's stateSchema */ public validateStateObject( stateObject: StateType ): void { + if (stateObject && stateObject.hasOwnProperty( 'elementID' ) && stateObject.hasOwnProperty( 'value' ) ) { + debugger; + } this.isStateObjectValid( stateObject, true ); } Index: phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts b/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts --- a/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts (revision c46c3b489a6495e0789881483cc5b0b8b9856cf1) +++ b/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts (date 1674684323664) @@ -31,7 +31,7 @@ new ConvertToStringsStateForLocale( /phScaleBasics\.global\.model\.solutes\.(.*)\.nameProperty/, 'phScaleBasics.general.model.strings.phScale.choice.$1StringProperty', stringsStatePhetioID ), new NumberPropertyNullRangeIsInfinite(), - new ConvertToStringsStateForLocale( 'phScaleBasics.macroScreen.nameProperty', 'phScaleBasics.general.model.strings.phScale.screen.macroStringProperty', stringsStatePhetioID ), + // new ConvertToStringsStateForLocale( 'phScaleBasics.macroScreen.nameProperty', 'phScaleBasics.general.model.strings.phScale.screen.macroStringProperty', stringsStatePhetioID ), new TandemFragmentRenamed( `${simName}.general.view.soundManager.enhancedSoundEnabledProperty`, `${simName}.general.view.soundManager.extraSoundEnabledProperty` ), new TandemFragmentRenamed( `${simName}.general.view.navigationBar.phetButton.phetMenu.enhancedSoundMenuItem.visibleProperty`, `${simName}.general.view.navigationBar.phetButton.phetMenu.extraSoundMenuItem.visibleProperty` ), Index: phet-io-wrappers/common/js/migration/MigrationEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/MigrationEngine.ts b/phet-io-wrappers/common/js/migration/MigrationEngine.ts --- a/phet-io-wrappers/common/js/migration/MigrationEngine.ts (revision 11c46bd47d74931c5b9f69baee5154ca9426c572) +++ b/phet-io-wrappers/common/js/migration/MigrationEngine.ts (date 1674684641948) @@ -60,6 +60,7 @@ this.migrationCheckers.forEach( checker => checker.call( this, command ) ); + command.args[ 0 ][ 'phScaleBasics.macroScreen.nameProperty' ].elementID = ''; const endTime = Date.now(); const elapsed = endTime - startTime; ```
zepumph commented 1 year ago

@samreid and I found 2 issues.

The first is that we were not correctly iterating up to the base case (supertype with no more supertype) correctly. This seems to have been a problem ever since we created stateSchema and merged it onto master.

The second issue is a hard failure in validating if the schema expects a "level" but the stateObject is undefined at that level.

zepumph commented 1 year ago

This changes the API, let's cherry pick this into ph-scale!!

zepumph commented 1 year ago

@samreid and I feel good about master. see https://github.com/phetsims/ph-scale/issues/279 for cherry picks in ph-scale.

zepumph commented 1 year ago

Oops. @samreid please note the newly needed migration rules here, since schema validation was fixed in this issue. We should definitely cherry pick that in https://github.com/phetsims/ph-scale/issues/279

zepumph commented 1 year ago

The above linked issues came out of CT results from this bug fix. Woo!! Both cases were obvious and easy oversights. I'm glad the assertion is working as expected. We may find more of these in the future when current validation:false phet-io sims turn on/test with validation.

zepumph commented 1 year ago

@samreid and I greatly improved upon my crappy rules committed back in https://github.com/phetsims/tandem/issues/290#issuecomment-1404355949. We made a single, more general rule that applies to the stateObject serialization for the whole PhET-iO type, not just by each single phetioID. We feel much much better about the work-in-progress.

zepumph commented 1 year ago

Note that AddStateElement is currently a hack, and next time we run into it, we should outfit it to be more general, and similar to IOTypeStateAttributeDelete.