Id's not null #15131

Skrypt · 2024-01-19T19:47:45Z

Related with #14786

Relevates that the ContentItemVersionId can be NULL which I think needs to be fixed.
Well at least our tests fails if we set this to NotNull. So basically when we create a new content item it should probably set a new UID all the time.

Notes

DefaultContentManager.RestoreAsync doesn't have any unit tests.

src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Migrations/OpenIdMigrations.cs

src/OrchardCore/OrchardCore.ContentManagement/Records/Migrations.cs

kevinchalet

👍🏻

src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Migrations/OpenIdMigrations.cs

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Skrypt · 2024-01-30T06:07:40Z

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

-            {
-                contentItem.ContentItemVersionId = _idGenerator.GenerateUniqueId(contentItem);
-                contentItem.Published = true;
-                contentItem.Latest = true;


We don't rely on a ContentItemVersionId not set on the ContentItem to define if Latest or not.

Have you tried the cloning events after this change? Or the code path in drivers that check for IsNew ? I have a weird feeling that is was there for a reason.

Other example:

public async Task<ContentValidateResult> RestoreAsync(ContentItem contentItem) { // Prepare record for restore. // So that a new record will be created. contentItem.Id = 0; // So that a new version id will be generated. contentItem.ContentItemVersionId = ""; contentItem.Latest = contentItem.Published = false;

which is fine as long as there is a unit test for this method than ensures that your changes are still fine.

Yeah I had the same thought. Remembered that all we do is set the ContentItemVersionId to String.Empty in some use cases which should be fine.

Cloning worked else the unit tests would have failed.

Restore seems to have issues.

Restore fixed

For CloningAsync and ClonedAsync the only difference is that now the ContentItemVersionId will be different than null. Should not be affecting our current implementations.

The only thing I'm not sure about is the IsNew code path in drivers. Right now, I'm assuming that some unit test would have already failed.

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Skrypt · 2024-01-30T06:11:13Z

It is probably missing a new UpdateTo for each modules Migration to alter their database. Though, it may not work if ContentItemVersionId's are null in that database. I would rather push this as a breaking change. I can change my mind on this but we don't know the many custom modules out there that relies on the ContentItemVersionId.

src/OrchardCore/OrchardCore.ContentManagement/Records/Migrations.cs

sebastienros · 2024-01-30T17:42:38Z

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

-            {
-                contentItem.ContentItemVersionId = _idGenerator.GenerateUniqueId(contentItem);
-                contentItem.Published = true;
-                contentItem.Latest = true;


Have you tried the cloning events after this change? Or the code path in drivers that check for IsNew ? I have a weird feeling that is was there for a reason.

src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Migrations/OpenIdMigrations.cs

Piedone

Do you all want to get back to this?

Also, while this is technically a breaking change, should it break anything in practice? Since null data is already wrong and shouldn't exist for these columns.

src/OrchardCore.Modules/OrchardCore.Alias/Migrations.cs

Piedone · 2024-03-22T19:56:21Z

src/OrchardCore.Modules/OrchardCore.AuditTrail/Migrations.cs

@@ -12,11 +12,11 @@ public class Migrations : DataMigration
        public async Task<int> CreateAsync()
        {
            await SchemaBuilder.CreateMapIndexTableAsync<AuditTrailEventIndex>(table => table
-                .Column<string>(nameof(AuditTrailEventIndex.EventId), column => column.WithLength(26))
+                .Column<string>(nameof(AuditTrailEventIndex.EventId), column => column.NotNull().WithLength(26))


Updates for all of these changes in this PR are also needed.

Skrypt · 2024-03-22T20:07:00Z

Yes, the breaking change is that you can't set a db column to non-null if there are null values in it. Basically, we would need to repopulate all the non-null uuid's by doing SQL Queries to self heal the DB before doing a schema update.

Piedone · 2024-03-22T20:32:41Z

I see so there can be null values, actually. OK, then indeed with some migrations this can be made non-breaking (or at least automatic, and only breaking if you rely on nullability on a lower level).

Skrypt · 2024-04-22T13:44:24Z

@Piedone This PR is one of those breaking changes that may be necessary for QOL in OC at some point. I can't push more time on it right now but it should definitely be merged as part of making OC more robust.

Piedone · 2024-04-22T13:47:58Z

It would be great, but it needs additional work to make it mergeable: #15131 (comment).

github-actions · 2024-07-02T02:01:11Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

github-actions · 2024-07-17T04:40:03Z

Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it.

Skrypt added 3 commits January 19, 2024 14:45

Id's not null

97a0956

Revert for ContainedContentItemId

ef0316e

Revert NotNull for ContentItemVersionId

30104c7

Skrypt commented Jan 19, 2024

View reviewed changes

src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Migrations/OpenIdMigrations.cs Show resolved Hide resolved

Skrypt commented Jan 19, 2024

View reviewed changes

src/OrchardCore/OrchardCore.ContentManagement/Records/Migrations.cs Outdated Show resolved Hide resolved

kevinchalet reviewed Jan 19, 2024

View reviewed changes

Skrypt added 2 commits January 19, 2024 22:07

Update based on Kevin recommendations

58b5ec5

Fix ContentItemVersionId Null issues

062d315

Skrypt commented Jan 30, 2024

View reviewed changes

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs Show resolved Hide resolved

Simplify

8b54a4c

Skrypt commented Jan 30, 2024

View reviewed changes

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs Show resolved Hide resolved

Skrypt marked this pull request as ready for review January 30, 2024 06:09

Skrypt requested a review from sfmskywalker as a code owner January 30, 2024 06:09

Skrypt added the breaking change 💥 Issues or pull requests that introduces breaking change(s) label Jan 30, 2024

Skrypt self-assigned this Jan 30, 2024

Skrypt requested a review from sebastienros January 30, 2024 17:20

sebastienros reviewed Jan 30, 2024

View reviewed changes

Skrypt added 6 commits January 30, 2024 14:03

Remove NotNull on AlterIndexTableAsync

a14b402

Revert change on AlterIndexTableAsync

09f045d

Fix restore

851d945

Add comment

c43e0f5

Better implementation

85bea57

Merge branch 'main' into skrypt/14786-ids-not-null

850e98a

github-actions bot mentioned this pull request Feb 1, 2024

Monthly community metrics report for 2024-01-01..2024-01-31 #15218

Closed

Skrypt added the don't merge label Feb 3, 2024

Piedone reviewed Mar 22, 2024

View reviewed changes

Merge branch 'main' into skrypt/14786-ids-not-null

d394d80

github-actions bot added the stale label Jul 2, 2024

github-actions bot closed this Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Id's not null #15131

Id's not null #15131

Skrypt commented Jan 19, 2024 •

edited

Loading

kevinchalet left a comment

Skrypt Jan 30, 2024

sebastienros Jan 30, 2024

Skrypt Jan 30, 2024

sebastienros Jan 30, 2024 •

edited

Loading

Skrypt Jan 30, 2024 •

edited

Loading

Skrypt Jan 31, 2024

Skrypt Jan 31, 2024

Skrypt Jan 31, 2024

Skrypt Jan 31, 2024

Skrypt Jan 31, 2024 •

edited

Loading

Skrypt commented Jan 30, 2024 •

edited

Loading

sebastienros Jan 30, 2024

Piedone left a comment

Piedone Mar 22, 2024

Skrypt commented Mar 22, 2024

Piedone commented Mar 22, 2024

Skrypt commented Apr 22, 2024

Piedone commented Apr 22, 2024

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 17, 2024

Id's not null #15131

Id's not null #15131

Conversation

Skrypt commented Jan 19, 2024 • edited Loading

Notes

kevinchalet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Skrypt Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skrypt Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Skrypt commented Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Piedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skrypt commented Mar 22, 2024

Piedone commented Mar 22, 2024

Skrypt commented Apr 22, 2024

Piedone commented Apr 22, 2024

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 17, 2024

Skrypt commented Jan 19, 2024 •

edited

Loading

sebastienros Jan 30, 2024 •

edited

Loading

Skrypt Jan 30, 2024 •

edited

Loading

Skrypt Jan 31, 2024 •

edited

Loading

Skrypt commented Jan 30, 2024 •

edited

Loading