DefaultContentManager.GetAsync(IEnumerable<string> contentItemIds) always updates documents in database

[frank1422]

When calling GetAsync(IEnumerable<string> contentItemIds) on DefaultContentManager it seems like all retrieved documents are updated in the database.

It happens for OrchardCore 2.0.0 (also for older versions when called with VersionOptions.Published)

I would expect that the documents are not updated, which is the case when calling GetAsync(string contentItemId) for each id.

Should

// We save the previous version further because this call might do a session query.
await _session.SaveAsync(item, checkConcurrency: true);

be inside

if (options.IsDraftRequired) {}

as in GetAsync(string contentItemId)?

[github-actions]

Thank you for submitting your first issue, awesome! 🚀 We're thrilled to receive your input. If you haven't completed the template yet, please take a moment to do so. This ensures that we fully understand your feature request or bug report. On what happens next, see the docs.

If you like Orchard Core, please star our repo and join our community channels.

[MikeAlhayek]

@sebastienros I am thinking the line can be remove. Thoughts?

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Lines 291 to 292 in b3c4b46

	// We save the previous version further because this call might do a session query.
	await _session.SaveAsync(item, checkConcurrency: true);

Because we are saving the item when IsDraftRequired is requested

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Lines 291 to 292 in b3c4b46

	// We save the previous version further because this call might do a session query.
	await _session.SaveAsync(item, checkConcurrency: true);

and on

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Line 302 in b3c4b46

await _session.SaveAsync(item, checkConcurrency: true);

I am not sure I understand the We save the previous version further because this call might do a session query. comment. But I tend to agree that L291-L292 should be removed.

[sebastienros]

Lines 291/292 can be removed because of line 302 as you correctly pointed.

// We save the previous version further because this call might do a session query.

Is a wrong copy-paste IMO

[MikeAlhayek]

@frank1422 thank you for logging this issue. I would love to see a PR from you! Do you think you can submit a PR by removing line 291/292?

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[sebastienros]

I assume you created the issue because you saw actual commit in the db, I am now wondering what caused it, since yessql would not create an update if the documents are the same. Or is it because we change a version property for instance?

On a similar note, is there a simple way to trigger this method call from the UI?

[frank1422]

@sebastienros I didn't check the database for actual commits, but we got at lot of ConcurrencyExceptions like

The document with Id '499' and type 'OrchardCore.ContentManagement.ContentItem, OrchardCore.ContentManagement.Abstractions' could not be updated as it has been changed by another process.

The problem started when we upgraded to Orchard Core 2.0.0 specifically on pages where we use a ContentPickerField.

I think the problem when using a ContentPickerField was introduced with 77c69c6. Before that point DefaultContentManager had a separate method (without options) to get multiple content items, which was used in ContentPickerField.cshtml.

I noticed the problem before, when retrieving multiple content items with GetAsync and VersionOptions.Published but thought it was by design and switched to using the version without options.

[frank1422]

When looking at the old code, I think that 77c69c6 introduced another problem, as the order of the content items are not guaranteed.