Change the Id and the DocumentId from int to long

[MikeAlhayek]

Fix #13997

This PR will introduce a very low risk of a breaking change since the IContentManagerSession.cs interface was modified from accepting int to accepting long. It is low risk because this interface should be only used by OC code. Also, if someone is using it to pass int it should be allowed which isn't going to break anything.

Okay... It turned out there is more breaking changes due to changes to interfaces.

[hishamco]

I'm okay with the changes, but I suggest removing anything unrelated to the actual change because JK is already has several PRs

[hishamco]

We might need to triage this

[MikeAlhayek]

I need to update more objects before we merge this. Added do not merge for now.

[jtkech]

So it seems that regardless of the current IdentityColumnSize, the Id columns of index tables are always an int under Sqlite and always a bigint under Sql server.

I'm looking at the code on YesSql side.

[MikeAlhayek]

Maybe line 20 in this file https://github.com/sebastienros/yessql/blob/main/src/YesSql.Provider.Sqlite/SqliteDialect.cs

[jtkech]

Hmm maybe, what I found is that it works if we use col => col.PrimaryKey().NotNull() as it is used in YesSql to create the Document table, but it doesn't work if we use col => col.PrimaryKey().Identity() as it is currently used for the IndexingTask table.

And I think it doesn't work if we use column => column.Identity().NotNull() as it is used on YesSql side for index tables, I will try this with the IndexingTask table.

[jtkech]

Yes, it works with .PrimaryKey() but not if we use .Identity() which is the case on YesSql for index tables.

[jtkech]

Okay good new, for sql server it works when we configure the DefaultIdentityColumnSize option in appsettings.json, and we don't need to modify the IndexingTask migration for the Id column because the option is correctly taken into account, so I will revert this change.

About Sqlite and PostgreSql, looks like using a bigint for an Identity column is not yet supported, on YesSql side through the dialect the same command string is used.

Sqlite

    public override string IdentityColumnString => "integer primary key autoincrement";
    public override string LegacyIdentityColumnString => "integer primary key autoincrement";

PostgreSql

    public override string IdentityColumnString => "SERIAL PRIMARY KEY";
    public override string LegacyIdentityColumnString => "SERIAL PRIMARY KEY";

While for SqlServer2 different strings are used.

    public override string IdentityColumnString => "[BIGINT] IDENTITY(1,1) primary key";
    public override string LegacyIdentityColumnString => "[INT] IDENTITY(1,1) primary key";

So nothing we can do for now ;)

[jtkech]

@MikeAlhayek

LGTM but still mising updates, check the below files (I may be wrong for some of them).

Hmm, looks like some are using the Id column as an identifier, some may need to be updated if related to data that we can export across databases, but in separate PRs ;)

[jtkech]

For now I cleaned up the releases doc before updating it.

Because it is related to more entities, interfaces and map indexes.

[MikeAlhayek]

@jtkech can you please search the solution for IEnumerable<int>? I think there is at least one more place in OC.Contents/AdminControler that require to be changed.

[MikeAlhayek]

@jtkech there is an instance in MenuPartDisplayDriver that needs to also be updated.

Also, I think we should list all the interfaces/domain models that were impacted in the "Breaking Change" section in the release notes of 1.7

Updated

The MenuPartDisplayDriver may not need to be changed since it seems to be parsing out a string coming from the UI.

[jtkech]

About MenuPartDisplayDriver not sure, I will check it this night.

Yes about the doc, I will list here the updated entities, interfaces and map indexes.

[jtkech]

@MikeAlhayek

Okay I updated some missing int to long, I updated the release notes, and I did some little tests, a fresh setup, creating a content item, a workflow and a deployment plan, I also tried to run it on an existing database having the same entities but created before the code changes.

I let you review the doc and maybe we should do more tests, in the meantime I will approve the PR.

[MikeAlhayek]

@jtkech I made minor changes to the release notes. Trying to do some testing but there seems to be an issue with the main branch.

I am getting

[jtkech]

The main branch merged in this PR, or the main branch itself?

I'm finishing something then I will look at it

[MikeAlhayek]

The issue is in the main branch and seems to be affecting this PR as well. I am guessing there is something recently merged in main which maybe causing this issue.

[jtkech]

Okay, will look at it soon.

[jtkech]

Just tried the last main branch, did a fresh Blog setup, for now works fine on my side?

Anything to repro?

Hmm, I will try this PR branch

Edited: Just did the same on this PR branch, for now works fine on my side?

[MikeAlhayek]

@jtkech strange. It sounds like a tenant specific issue "maybe a specific feature". The issue seems to happen when logging in. Its seems to only happen to few sites but not all of them. I'll try to get more info tomorrow.

[jtkech]

Okay, when I switch to different branches sometimes I need to do git clean -xdf.

[MikeAlhayek]

Yes and we also have npm run cleanup. I usually use npm run cleanup since it only deletes bin and the lib folders. It'll keep things like App_Data. But with the npm run cleanup some tenants were experiencing the problem. I executed git clean -xdf all untracked files including App_Data were removed, a fresh tenants had no issue. I am just wondering what caused the issue I had earlier.

Anyhow, I tested this PR again and everything seems to be working as expected. I created new site with Int32 as the default and then switched to the code in this PR and everything seems to be smooth. This may be a hard one to triage without trying it like you and I did. If you think it is safe to merge it. Go for it.

[jtkech]

Okay let's go, I will merge it ;)

Also related to YesSql, take a look at this "issue" #14036 about the table schema, I understood why, see my comment #14036 (comment), but not sure something should be done on YesSql side, if not we may have to adapt our tenant validation, but not so urgent I think.

[MikeAlhayek]

I saw the issue with the schema. We discussed it today in the Steering meeting and Sebastian commented with recommendation.