chore: Merge master into next

[imnotjames]

This merges master into the next branch with a handful of extra commits to fix tests and other issues.

Changes I had to make to get the tests to pass:

commit	change
2c9315e	update FindOptionUtils to fix typing
0f292e3	update test to handle withDeleted living under FindOptions.optionswit…
cf15e3f	default import for Observable
f46c8a3	work around typing preventing test 3416 from running
d2201ab	fix test 6399
cbfbc56	SchemaTransformer requires a connection now
51da0ab	correctly handle composite PK & failed entity loads
0128a9e	FindCriteriaNotFoundError must extend EntityColumnNotFound
589c543	fix findOneOrFail passing invalid data to findOne
c88c863	drop generic from concrete EntityManagers
774c5ba	update 4415 to address cockroachDB changes

[imnotjames]

Merged master a second time - mostly to get the oracle fixes in so those tests pass. Once all the tests pass I'll iterate through the changes I made to ensure the tests pass between master and next

[imnotjames]

Looks like something in the next branch causes the query pattern to be drastically different in the next branch - not just this PR, either.

Master:

SELECT DISTINCT "distinctAlias"."User_primaryKey" as "ids_User_primaryKey" FROM (SELECT "User"."primaryKey" AS "User_primaryKey", "User"."email" AS "User_email", "User__access_token"."primaryKey" AS "User__access_token_primaryKey", "User__access_token"."userPrimaryKey" AS "3115cc2fef583c2761dee1374a5720" FROM "user" "User" LEFT JOIN "access_token" "User__access_token" ON "User__access_token"."userPrimaryKey"="User"."primaryKey" WHERE "User"."email" = :1) "distinctAlias"  ORDER BY "User_primaryKey" ASC FETCH NEXT 1 ROWS ONLY

SELECT "User"."primaryKey" AS "User_primaryKey", "User"."email" AS "User_email", "User__access_token"."primaryKey" AS "User__access_token_primaryKey", "User__access_token"."userPrimaryKey" AS "3115cc2fef583c2761dee1374a5720" FROM "user" "User" LEFT JOIN "access_token" "User__access_token" ON "User__access_token"."userPrimaryKey"="User"."primaryKey" WHERE ( "User"."email" = :1 ) AND ( "User"."primaryKey" IN (1) )

next branch (with & without merged in master)

SELECT "User"."primaryKey" AS "User_primaryKey", "User"."email" AS "User_email" FROM "user" "User" WHERE ("User"."email" = :1) FETCH NEXT 1 ROWS ONLY

SELECT "AccessToken"."primaryKey" AS "AccessToken_primaryKey", "AccessToken"."userPrimaryKey" AS "AccessToken_userPrimaryKey" FROM "access_token" "AccessToken" WHERE "AccessToken"."userPrimaryKey" IN (1)

SELECT "AccessToken"."primaryKey" AS "AccessToken_access_token_primaryKey", "AccessToken"."userPrimaryKey" AS "User_primaryKey" FROM "access_token" "AccessToken" WHERE "AccessToken"."userPrimaryKey" IN (1)

That's causing the oracle failures.. I'm thinking of just disabling this test but..

[imnotjames]

I attempted to skip that test but it seems like it just breaks in a test that also runs through that behavior. The area that's causing this behavior is:

typeorm/src/query-builder/RelationIdLoader.ts

Lines 291 to 298 in 1e1595e

	relation.entityMetadata.primaryColumns.forEach(primaryColumn => {
	const columnName = primaryColumn.entityMetadata.name + "_" + relation.inverseRelation!.propertyPath.replace(".", "_") + "_" + primaryColumn.propertyPath.replace(".", "_");
	qb.addSelect(mainAlias + "." + primaryColumn.propertyPath, columnName);
	});
	relation.joinColumns.forEach(column => {
	const columnName = column.referencedColumn!.entityMetadata.name + "_" + column.referencedColumn!.propertyPath.replace(".", "_");
	qb.addSelect(mainAlias + "." + column.propertyPath, columnName);
	});

I tried to use column.propertyAliasName instead of the madness that it uses there, but that breaks other tests where they can't associate the data into the related models. Probably somewhere else that looks for these.

Path forward right now is to dig into that further, some minor refactoring of the RelationIdLoader & others so it uses something that will listen to the alias values - add that commit to next & also to a PR for master.

Branch with just that change.

[imnotjames]

We could just merge it as is - eg, with the failing oracle test. I'm pretty sure the failing oracle tests are due to the next branch & not the merge. I saw similar results running the oracle tests against the upstream next branch as well.

@pleerock thoughts?

PS I updated the body of the PR to include the extra commits I created for it and why

[pleerock]

Thank you @imnotjames . Let's merge, I'll release a new next version and let's keep track on newly reported issues (there are not so many next users afaik). Also, what do we need to do to fix oracle issues on next ?

[pleerock]

according to CI, ORA-00972: identifier is too long is an error. Looks like we need to enable logging, check which query fails and where identifier isn't shortened?

[imnotjames]

Also if you'd like tos we the queries that are causing this they're in a comment above.

[pleerock]

oh, okay.. so, for your question on why it creates different queries - because in next branch have a different approach on loading relations - instead of using joins, it creates creates and runs separate queries.

on question why it fails - you probably remember that Oracle has a limitation on length of the identifiers - aliases and other similar symbols. So, my assumption is we don't shorten alias here, that's why we have an error.

Maybe placing a following code (taken from FindOptionsUtils`):

            let relationAlias: string = alias + "__" + relation;
            // shorten it if needed by the driver
            if (qb.connection.driver.maxAliasLength && relationAlias.length > qb.connection.driver.maxAliasLength) {
                relationAlias = shorten(relationAlias);
            }

in the code you provided can fix the issue.

[imnotjames]

on question why it fails - you probably remember that Oracle has a limitation on length of the identifiers - aliases and other similar symbols. So, my assumption is we don't shorten alias here, that's why we have an error.

Right - I had made that change, more or less, and found that while we didn't get the issue with oracle, elsewhere we had a failure with the alias being expected as what it previously was.

The amount of change needed to correct the issue started to feel a bit out of the scope for this PR - as the test was failing silently before, and it seems like this code should have been an issue before as well.

[pleerock]

Okay, let's merge it. Thank you a lot

[thynson]

Will there be a new rc release? Can't wait for it!

[pleerock]

released in typeorm@0.3.0-rc.20

[nebkat]

Was this mistakenly merged as a squash? imnotjames@b2eeba6 correctly maintains commit history from both branches but 903765d does not. next...imnotjames:next shows the missing commits.

This is a problem for merging between the two branches now, as next...master shows commits since April 13th, while imnotjames/typeorm@next...typeorm:master shows only from after September 26th.

[pleerock]

@nebkat I'm not sure... what is a proper resolution? revert changes and merge once again?

[nebkat]

If you're willing to accept a broken history on the next branch the best resolution would be to hard reset before the squash happened and then do the merge. I don't think there are many pull requests for next so doubt a lot of people would be affected (most importantly master will remain good).

With revert it won't break history for those who have a local copy of the next branch, but when looking through git blame/history for a file it will be annoying in future.

I can do either option on my fork if you want?

[nebkat]

I thought you had done some additional changes on top of @imnotjames' commit but I see it is identical just squashed. If you are OK with the hard reset/broken history route I have pushed it here nebkat@28688b8.