Stop including the discriminator in the JSON id by default

[ajcvickers]

Fixes #34179

There are also new APIs on the entity type and model builders to facilitate optional behaviors, including reverting to the pre-9 behavior:

• Entity().IncludeDiscriminatorInJsonId();
• Entity().IncludeRootDiscriminatorInJsonId();
• Entity().AlwaysCreateShadowIdProperty();

Note that this change requires regeneration of the Northwind database.

[AndriySvyryd]

How do we make sure that the Northwind database is recreated on the CI with the new values and is also correctly recreated with the old values when code from an older branch runs?

[roji]

How do we make sure that the Northwind database is recreated on the CI with the new values and is also correctly recreated with the old values when code from an older branch runs?

IMHO that's another good reason to not rely on a persistent database in CI, but rather to just use the emulator, where it's guaranteed to be recreated each time (I think we agreed we want to go in that direction anyway, possibly except for specific RBAC tests which can't be executed with the emulator?).

If we do want to stay with non-emulator tests, we could simply recreate the database each time, but on Ci only (to avoid making local runs slow), e.g. by checking the CI environment variable.

[AndriySvyryd]

IMHO that's another good reason to not rely on a persistent database in CI, but rather to just use the emulator, where it's guaranteed to be recreated each time (I think we agreed we want to go in that direction anyway, possibly except for specific RBAC tests which can't be executed with the emulator?).

We need to do #19973 first

If we do want to stay with non-emulator tests, we could simply recreate the database each time, but on Ci only (to avoid making local runs slow), e.g. by checking the CI environment variable.

That would still not work for tests from old branches. We'd need to rename the database.

[ajcvickers]

@roji @AndriySvyryd New version up attempting to follow Andriy's guidance.

[ajcvickers]

@roji This is now updated with fixes for discriminator checking and more tests. Can you check the test baselines to make sure you think we are doing ReadItem or not as appropriate? I commented on each test case where we don't do ReadItem indicating why not for that case.

[roji]

Hey, here's a review of the ReadItemPartitionKey tests only. The actual behavior seems correct to me (i.e. I haven't noticed a case where we do ReadItem when we shouldn't or vice versa). So below see mainly some comments about the structure/factoring of these tests.

As a general comment, there's maybe a bit more systematic duplication of tests across various scenarios than I'd have personally done... This is definitely a matter of personal style/taste on how to test: more black box-style "cover all permutations" vs. more white-box "think about specific scenarios you want/need to cover" (I lean much more in the latter direction). For example, do we think we need all the duplication along the only/non-only axis, as well as everything on the leaf. I'm also noting that there seem to be relatively few tests which actually differ across the different implementations (e.g. HierarchicalPartitionKeyEntity is practically identical across all versions).

But I think it's all OK, hopefully it doesn't cost us in terms of maintainability/complexity.

Regardless FYI it took me a while to understand all the test factoring here and realize what extends from what and why :) It makes sense to me now, but maybe a comment on each test class to explain what it's there for could be useful for our future selves.

[roji]

Any specific reason to do this on this specific test suite (not pushing back)?

[ajcvickers]

Because otherwise the test data results in conflicts in the JSON id values.

[roji]

Given we're gonna have so many ReadItemPartitionKey query tests, maybe they deserve their own sub-folder...

[ajcvickers]

Are you asking me to put them in a subfolder? Happy to do so, if that's what you want.

[roji]

Yeah that was the idea

[roji]

Also just additional things in predicate

[ajcvickers]

As a general comment, there's maybe a bit more systematic duplication of tests across various scenarios than I'd have personally done... This is definitely a matter of personal style/taste on how to test: more black box-style "cover all permutations" vs. more white-box "think about specific scenarios you want/need to cover" (I lean much more in the latter direction).

I very much believe that if I only do "white box" testing, then I will ship significantly more bugs. This is based on my experience of writing tests in this way and frequently finding a bug in a scenario I had never even thought of. So, at least for people of my ability, this will be a trade off for the advantages of having fewer tests against us shipping more bugs.

[AndriySvyryd]

There aren't any blocking issues