Skip to content
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.

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

Wrong property tags issue #970

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Abhijeet-Bentley
Copy link

@Abhijeet-Bentley Abhijeet-Bentley commented Jan 17, 2025

Issue Link: iTwin/itwinjs-core#7473

  • Added tests to check for code's behaviour if wrong property tags are encountered
  • Expected behavior on encountering wrong property tag:
  1. Older version: Default to string
  2. Current/Latest version: Throws ERROR
  3. Future version: Default to string
  • Added code fix which takes care of all above scenarios.

@@ -44,6 +44,11 @@ SchemaReadStatus ReadTypeNameWithPruning(pugi::xml_node node, ECPropertyP prop,
// must come after prune check to make sure we don't default to string properties that should be pruned
if (setTypeStatus == ECObjectsStatus::ParseError && ignoreParseErrors)
{
// Unknown type encounter in the schema for the current version returning ERROR status
if (!(prop->GetClass().GetSchema().OriginalECXmlVersionGreaterThan(ECVersion::Latest)) && !(prop->GetClass().GetSchema().OriginalECXmlVersionLessThan(ECVersion::Latest)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just check for ECVersion::Latest ?

@CLAassistant
Copy link

CLAassistant commented Jan 17, 2025

CLA assistant check
All committers have signed the CLA.

ASSERT_EQ(deserializationStatus, schema.IsValid()) << "Test case number: " << testCaseNumber << " failed due to invalid schemas.";

// Checking if the wrong property tags are defaulted to string type
if (testCaseNumber != 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner and more readable if you check deserializationStatus instead of the test case number.

if (testCaseNumber == 5)
{
// Schema import should fail when ECXml version of the schema is greater than the current version that the ECDb supports
ASSERT_EQ(false, m_ecdb.Schemas().ImportSchemas(context->GetCache().GetSchemas()) == SchemaImportResult::OK) << "Test case number " << testCaseNumber << " failed since, the schema was imported.";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure the import schema is behaving the way we want it for the different versions.
Also,
ASSERT_EQ(m_ecdb.Schemas().ImportSchemas(context->GetCache().GetSchemas()), SchemaImportResult::OK / SchemaImportResult::ERROR)

@Abhijeet-Bentley Abhijeet-Bentley changed the title iModel Native/wrong property tags issue Wrong property tags issue Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants