Skip to content

WIP - Major change to dynamic schema fails schema import #8164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

WIP - Major change to dynamic schema fails schema import #8164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
40983c5
Added tests
RohitPtnkr1996 May 30, 2025
ec15f2d
Added changelog
RohitPtnkr1996 May 30, 2025
f117033
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 May 30, 2025
b92a25e
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 May 31, 2025
9124086
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jun 3, 2025
922e5ed
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jun 4, 2025
0b25645
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jun 12, 2025
25c46c3
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jun 13, 2025
1d5e1ca
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jun 16, 2025
5bea24c
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jun 17, 2025
29a1fa6
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jul 10, 2025
16d01b5
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jul 11, 2025
7c5c291
Merge branch 'master' into rohitptnkr/major-schema-upgrade-investigation
RohitPtnkr1996 Jul 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...s/@itwin/core-backend/rohitptnkr-major-schema-upgrade-investigation_2025-05-30-09-25.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@itwin/core-backend",
"comment": "",
"type": "none"
}
],
"packageName": "@itwin/core-backend"
}
99 changes: 98 additions & 1 deletion core/backend/src/test/imodel/IModel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3119,7 +3119,7 @@ describe("iModel", () => {
imodel.close();
});

function createElemProps(_imodel: IModelDb, modId: Id64String, catId: Id64String, className: string): GeometricElementProps {
function createElemProps(_imodel: IModelDb, modId: Id64String, catId: Id64String, className: string): GeometricElementProps {
// Create props
const elementProps: GeometricElementProps = {
classFullName: className,
Expand Down Expand Up @@ -3303,4 +3303,101 @@ describe("iModel", () => {
}
testImodel.close();
});

function assertSchemaVersion(imodel: IModelDb, schemaName: string, expectedVersion: string) {
const schemaProps = imodel.getSchemaProps(schemaName);
assert.isDefined(schemaProps);
assert.strictEqual(schemaProps.version, expectedVersion);
}

async function assertClassProperty(imodel: IModelDb, classNameFullName: string, properties: any) {
const testClass = await imodel.schemaContext.getSchemaItem(classNameFullName, EntityClass);
assert.isDefined(testClass);
for (const prop of properties) {
const property = await testClass?.getProperty(prop.propName);
assert.equal(prop.propertyExists, property !== undefined, `Property ${prop.propName} existence check failed for class ${classNameFullName}`);
}
}

it("Major schema updates should only work for dynamic schemas", async () => {
const imodelPath = IModelTestUtils.prepareOutputFile("IModel", "majorschemaupgrade.bim");

// Remove the file if it already exists
if (IModelJsFs.existsSync(imodelPath))
IModelJsFs.unlinkSync(imodelPath);

// Create a new empty snapshot iModel
const testImodel = SnapshotDb.createEmpty(imodelPath, { rootSubject: { name: "majorschemaupgrade" } });

// Import initial schema and verify version
await testImodel.importSchemaStrings([`<?xml version="1.0" encoding="utf-8" ?>
<ECSchema schemaName="TestSchema" alias="ts" version="1.0.0" xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.2">
<ECSchemaReference name="BisCore" version="1.0.0" alias="bis"/>

<ECEntityClass typeName="TestClass" >
<BaseClass>bis:PhysicalElement</BaseClass>
<ECProperty propertyName="PropToDelete" typeName="string" />
</ECEntityClass>
</ECSchema>`]);
assertSchemaVersion(testImodel, "TestSchema", "01.00.00");
await assertClassProperty(testImodel, "TestSchema.TestClass", [{ propName: "PropToDelete", propertyExists: true }]);

// Attempt to delete a property 'PropToDelete' in a major schema update on a non-dynamic schema (should fail)
try {
await testImodel.importSchemaStrings([`<?xml version="1.0" encoding="utf-8" ?>
<ECSchema schemaName="TestSchema" alias="ts" version="2.0.0" xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.2">
<ECSchemaReference name="BisCore" version="1.0.0" alias="bis"/>

<ECEntityClass typeName="TestClass" >
<BaseClass>bis:PhysicalElement</BaseClass>
</ECEntityClass>
</ECSchema>`]);
assert.fail("Expected an error to be thrown when trying to delete a property in a major schema update on a non-dynamic schema.");
} catch {
// Confirm schema version and property still exist
assertSchemaVersion(testImodel, "TestSchema", "01.00.00");
await assertClassProperty(testImodel, "TestSchema.TestClass", [{ propName: "PropToDelete", propertyExists: true }]);
}

// Make the schema dynamic and verify version and properties
await testImodel.importSchemaStrings([`<?xml version="1.0" encoding="utf-8" ?>
<ECSchema schemaName="TestSchema" alias="ts" version="1.0.1" xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.2">
<ECSchemaReference name="BisCore" version="1.0.0" alias="bis"/>
<ECSchemaReference name="CoreCustomAttributes" version="1.0.0" alias="CoreCA" />

<ECCustomAttributes>
<DynamicSchema xmlns = 'CoreCustomAttributes.1.0.0' />
</ECCustomAttributes>

<ECEntityClass typeName="TestClass" >
<BaseClass>bis:PhysicalElement</BaseClass>
<ECProperty propertyName="PropToDelete" typeName="string" />
<ECProperty propertyName="AnotherProperty" typeName="int" />
</ECEntityClass>
</ECSchema>`]);
assertSchemaVersion(testImodel, "TestSchema", "01.00.01");
await assertClassProperty(testImodel, "TestSchema.TestClass", [{ propName: "PropToDelete", propertyExists: true }, { propName: "AnotherProperty", propertyExists: true }]);

// Now try to delete a property 'PropToDelete' in a major schema update on a dynamic schema (should succeed)
try {
await testImodel.importSchemaStrings([`<?xml version="1.0" encoding="utf-8" ?>
<ECSchema schemaName="TestSchema" alias="ts" version="2.0.1" xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.2">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this did not work as well. But my understanding is that dynamic schema does not require major version on schema to change to make major version change. That why we have flag AllowMajorSchemaUpgradeForDynamicSchemas other wise why we have this flag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note another case is that the stored schema that got upgraded was of ECXML 2.0

Id Name         DisplayLabel Description Alias VersionDigit1 VersionDigit2 VersionDigit3 OriginalECXmlVersionMajor OriginalECXmlVersionMinor 
-- ------------ ------------ ----------- ----- ------------- ------------- ------------- ------------------------- ------------------------- 
42 OpenPlant_3D OpenPlant_3D (null)      op3d              1             0          3843                         2                         0

Copy link
Contributor

@khanaffan khanaffan May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I exported and then imported it again with delete properties. I did follow two both failed where i got error that schema upgrade failed.

  1. I exported from iModelConsole it seems to export with ECXML.3.1
  2. I exported from itwin.js using exportSchemas() it exported with ECXML 3.2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this helps you reproduce it. I have a dataset and schema on disk so maybe you can use that to reproduce it.

Copy link
Contributor Author

@RohitPtnkr1996 RohitPtnkr1996 May 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this did not work as well. But my understanding is that dynamic schema does not require major version on schema to change to make major version change. That why we have flag AllowMajorSchemaUpgradeForDynamicSchemas other wise why we have this flag.

Not quite.
We have a blanket ban on all major version changes with the DisallowMajorSchemaUpgrade flag in iModelPlatform.
We relaxed this only for dynamic schemas.
The flag "AllowMajorSchemaUpgradeForDynamicSchemas" (defaults to true) was just to have some added control over dynamic schema major upgrades to disable/re-enable it in the future.

However, the general rules of major version change like incrementing read version are still in effect for all dynamic and non-dynamic schemas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the change to allow dynamic schemas to make major changes without requiring a read version increment.
Native PR: iTwin/imodel-native#1144

<ECSchemaReference name="BisCore" version="1.0.0" alias="bis"/>
<ECSchemaReference name="CoreCustomAttributes" version="1.0.0" alias="CoreCA" />

<ECCustomAttributes>
<DynamicSchema xmlns = 'CoreCustomAttributes.1.0.0' />
</ECCustomAttributes>

<ECEntityClass typeName="TestClass" >
<BaseClass>bis:PhysicalElement</BaseClass>
<ECProperty propertyName="AnotherProperty" typeName="int" />
</ECEntityClass>
</ECSchema>`]);
assertSchemaVersion(testImodel, "TestSchema", "02.00.01");
await assertClassProperty(testImodel, "TestSchema.TestClass", [{ propName: "PropToDelete", propertyExists: false }, { propName: "AnotherProperty", propertyExists: true }]);
} catch {
assert.fail("Expected no error to be thrown when deleting a property in a major schema update on a dynamic schema.");
}
});
});