iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
620 stars 210 forks source link

Regression 4.9.2 where aliased ECClassId switch from returning classId to className (backport #7279) [release/4.9.x] #7282

Closed mergify[bot] closed 3 weeks ago

mergify[bot] commented 3 weeks ago
  1. CTE query returning classid switch from className to classId in 4.9.x. This broke a user.
  2. PR 7235 Fix CTE, but this requires properly handling extendType information. This result in another break in behavior where aliased classid property was return as className. In 4.8.x/ 4.9.0 it used to return Id not class name. This break only happens for Concurrent Query for ECSqlStatement was fine as it set a flag called DoNotConvertClassIdsToClassNamesWhenAliased to reproduce previous behavior.
  3. This PR also set DoNotConvertClassIdsToClassNamesWhenAliased for concurrent query to reproduce behavior of classId returning class name when aliased to be compilable with 4.8.x

First query that broke user is following

SELECT t(aClassId) AS (SELECT ECClassId FROM Bis.Element) SELECT * FROM t
-- 4.8.x return className
-- 4.9.0 return classId
SELECT ECClassId AS aClassId FROM Bis.Element
--4.9.0 returned classId
--4.9.2 return className

After current fix

SELECT t(aClassId) AS (SELECT ECClassId FROM Bis.Element) SELECT * FROM t
-- return className as in 4.8.x
SELECT ECClassId AS aClassId FROM Bis.Element
-- returned classId  as in 4.8.x

imodel-native: https://github.com/iTwin/imodel-native/pull/894


This is an automatic backport of pull request #7279 done by Mergify.

mergify[bot] commented 3 weeks ago

Cherry-pick of 0716d56cbd51889e7ecfdfcf8fcd690b70b6bfd6 has failed:

On branch mergify/bp/release/4.9.x/pr-7279
Your branch is up to date with 'origin/release/4.9.x'.

You are currently cherry-picking commit 0716d56cbd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
    new file:   common/changes/@itwin/core-backend/affanK-regression_classid_2024-10-18-19-55.json
    modified:   core/backend/src/test/ecdb/ECSqlQuery.test.ts
    modified:   core/backend/src/test/ecdb/ECSqlStatement.test.ts

Unmerged paths:
  (use "git add <file>..." to mark resolution)
    both modified:   common/config/rush/pnpm-lock.yaml
    both modified:   core/backend/package.json
    both modified:   test-apps/display-test-app/android/imodeljs-test-app/app/build.gradle
    both modified:   test-apps/display-test-app/ios/imodeljs-test-app/imodeljs-test-app.xcodeproj/project.pbxproj
    both modified:   tools/internal/ios/core-test-runner/core-test-runner.xcodeproj/project.pbxproj

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

hl662 commented 3 weeks ago

@khanaffan, the tests in the CI builds are failing, showing the same errors - can you investigate?

khanaffan commented 3 weeks ago

@khanaffan, the tests in the CI builds are failing, showing the same errors - can you investigate?

so, I build both branches locally did not see any failure. I bet it might be wrong addon pull into this PR.