Depict null values in array type ECProperty output #6948
Conversation
…itwinjs-core into Sarunas/array-null-values
…be null as it's not yet revised
|
|
||
| - A `point2d` type property must always have all its coordinates present, hence updating a point2d to be partially null is not a valid input and would result in the property not being updated. This is why updating `p2dProp` to `{ x: null, y: 2.5 }` would be invalid and take no effect on the element's property. | ||
| - A `struct` type property can have all of their children properties set to null. The child property would end up being cleared from the parent object with the remaining properties remaining intact. This means that if `structProp` would include `{ doubleProp: 41.0, stringProp: null }`, the string property would be cleared with the doubleProp successfully updated to the expected value. | ||
| - An `array` type (primitive or struct) property can store null values as any other value of the respective type. It would happen to be represented alongside the ordinary properties as null. E.g. updating `arrBoolProp` to `[false, true, null, false]` is a perfectly valid data entry for a boolean type array property. |
There was a problem hiding this comment.
This is the behavior that has been controversial. @pmconne
There are three possible behaviors:
1 - accept the array as input and persist including the nulls.
2 - consider the array invalid if it has any null inputs.
3 - silently prune the null entries in the array, e.g. persist [false, true, false]
I do not think it's acceptable to prune the nulls from the array so only options 1 and 2 are OK.
I think #1 is the best option but I could be convinced to accept #2 if we're sure no one will ever want to be able to have a sparse array or have a fixed length array where the use null to indicate that an entry is not yet set.
There was a problem hiding this comment.
If we permit the possibility of sparse arrays then everywhere we define a TypeScript interface representing any array, we must define it as Array<T | undefined>~ instead of T[]`. We certainly don't do that today, and I don't think that it would make anyone's life better.
If sparse arrays became a requirement at some point, couldn't we consider a custom attribute permitting null entries, similar to how we have IsNullable for primitive properties?
There was a problem hiding this comment.
In theory, these changes only solve the fact that if nulls are already present in the DB, we display the data accurately as we have it. The sparse arrays have already been possible, it's just that we have been using the 3rd behavior that Colin has described above for representation, while storing the data into the respective database columns as has been input for us including all of the nulls and that seems like a bug.
we must define it as
Array<T | undefined>instead ofT[]
Is it a must to match all of the TypeScript interfaces to what exactly can be stored on the native side immediately? Given the business justification some time in the future, the only needed change would be switching up the interfaces on the TS side. If the interfaces are kept as is and everyone were strict about using them, this behavior should not ever be hit (though iTwin/itwinjs-backlog#1063 suggests it's not as uncommon) and the nulls wouldn't ever be persisted in the database, making this whole problem redundant.
There was a problem hiding this comment.
Over a discussion, we agreed to run a SQL script through iQuacs to count null data entries in array properties, thus determining whether there is actual usage of sparse arrays. If there is any, it was agreed to move forward with these changes and fix the currently undocumented null stripping from output, and if not - throw an error on finding a null inside an array.
…itwinjs-core into Sarunas/array-null-values
imodel-native: iTwin/imodel-native#806
Covering the expected behavior with tests, repurposed a test fixture so that it can make room for test cases not limited to struct arrays.
Appending the null behavior documentation with the agreed-on correct behaviors.
Closes iTwin/itwinjs-backlog#1162