Deprecate ECSqlStatement class and UseJsPropertyName and convertClassIdsToClassNames flags

ColinKerr commented 6 days ago

Why

There are multiple code paths to execute ECSql queries, they produce inconsistent results and have behavior that is undocumented.

What

The functions withPreparedStatement and withStatement will be deprecated along with the ECSqlStatement that they return. This API has been chosen for deprecation because it is synchronous, inconsistent by default and only available in the backend. The alternative (ECSqlReader) is consistent with default options, is async and consistent on the frontend and backend APIs.

The query option UseJsPropertyNames will be deprecated. It produces inconsistent results especially around conversion of class ids to class names. UseECSqlPropertyIndexes is a more performant alternative if you want to access the results by index and UseECSqlPropertyNames produces more consistent results that UseJsPropertyNames. See QueryRowFormat for more details.

The query option convertClassIdsToClassNames will be deprecated. It is currently inconsistently applied when used in conjunction with UseJsPropertyNames. It should be replaced with explicit use of the ec_classname function to convert class ids to class names. See QueryOptions for more details.

Impact

This change will impact anyone who uses withPreparedStatement or withStatement and anyone who uses ECSqlReader AND has used one of the flags UseJsPropertyNames or convertClassIdsToClassNames. For users of ECSqlReader updating to these changes is straight forwards and documentation + examples will be provided. For users of ECSqlStatement the changes are larger because the replacement API is synchronous.

Feedback Needed

We are interested in understanding use cases where synchronous queries executed on the main thread are necessary. This type of query can hang the backend freezing an electron app or making a server unresponsive. So we want to move the default away from sync queries but understand that there may be reasons to opt into a sync query but want to understand those before implementing a solution.

The removal of convertClassIdsToClassNames takes away a feature without providing a like replacement. We have seen this automatic replacement cause confusion over the years. We think an explicit ECSql function call is a better option because it removes this confusion while still being easy to use.

aruniverse commented 6 days ago

I imagine this will also encompass removing doNotConvertClassIdsToClassNamesWhenAliased ? https://github.com/iTwin/imodel-native/blob/b6eb90cf25aad470cc7e7b9cb419312806fbc0b9/iModelJsNodeAddon/api_package/ts/src/NativeLibrary.ts#L361

Internal usage via GH queries ECSqlStatement withPreparedStatement withStatement UseJsPropertyNames convertClassIdsToClassNames

khanaffan commented 5 days ago

Details on what was wrong and how to do it right

There are two ways to read value for row return by ECSqlStatement (Less frequently used API)

Get value by index of property in ECSqlStatement

In the following, we use getValue() to read a specific value that we choose in query. This Api use index to access select property. stmt.getValue(0).getId() will implicitly convert values.

        iModel.withStatement("SELECT ECInstanceId, CodeValue FROM BisCore.Element WHERE ECInstanceId=?", (stmt: ECSqlStatement){
          stmt.bindId(1, "0x1");
          if (stmt.step() == DbResult.BE_SQLITE_ROW) {
            // Use getValueXXXX function to get the value
            const id = stmt.getValue(0).getId();
            const codeValue = stmt.getValue(1).getString();
            return { id, codeValue };
          }
        });

Get value by converting row to javascript object using `getRow()`. (More frequently used API)

This method automatically transforms row using the (rules)[https://www.itwinjs.org/learning/ecsqlrowformat/]. There is some addition rule e.g. if XXXXClassId property is aliased in ECSQL then we return hex id of class

Note: This rules initially done to help more ts friendly. But caused more confusion than improve usability.

        iModel.withStatement("SELECT ECInstanceId, CodeValue FROM BisCore.Element WHERE ECInstanceId=?", (stmt: ECSqlStatement){
          stmt.bindId(1, "0x1");
          if (stmt.step() == DbResult.BE_SQLITE_ROW) {
            const row = stmt.getRow();
            const id = row.id;
            const codeValue = row.codeValue;
            return { id, codeValue }; // same as `return row`
          }
        });

In summary, two transformations are applied to row

rename system properties.
convert ECClassId to className for system properties.

We continue to have issue supporting this format because beside the confusion it also has some bugs that were use as features and fixing them was impossible without breaking users queries. Also, confusion of automatic conversion of name and values lead to logical bugs. e.g. since we convert ECInstanceId to Id automatically, people presumed their class has property id and so they might query like WHERE Id=? or WHERE className='BisCore.Element' which would failed to parse. These inconsistencies are a source of confusion and bugs.

ECSql API should return exactly what user selected. This is why we have ECSqlReader API which initially has to be backward compatible and thus has a flag to reproduce behavior as ECSqlStatement.getRow(). But it also implements the standard that we like to move to which is to return what ever user selected "as is" and let user control transformation.

How to use ECSqlReader and ECSql in the right way

A good way to use ECSqlReader is to access current row by index. It's optimal in all known ways and user in case need to create object from it can do it by himself and name the property in way they desire and meet linting rules. (ECSql is case-insensitive).

The code for ECSqlReader API is async can be called from frontend & backend. And is very simple in term of usability.

    const reader = iModel.createQueryReader(
      "SELECT ECInstanceId, CodeValue FROM BisCore.Element WHERE ECInstanceId=?",
      QueryBinder.from(["0x1"])
    );
    for await (const row of reader) {
      return { id: row[0], codeValue: row[1] };
    }

If user do not want to use index, they can also access current row by property Name (case-insensitive way)

    for await (const row of reader) {
      return { id: row.ecInstanceId, codeValue: row.codeValue };
    }

But end goal is user to create object from row by themselves and not depend on automatic conversion. User can alias properties to name them right or use reader.row.<case-insensitive-property-name> or use reader.row[<index>]. Index access being preferred way.

iTwin / itwinjs-core