api: don't override next version with new draft #232
Conversation
| @@ -199,8 +199,8 @@ def post_create(self, record): | |||
| # The parent record is created on pre_create. | |||
| versions = self.obj(record) | |||
| if self._create and self._set_next: | |||
| # if fork is none - it's a new? | |||
| versions.set_next() | |||
| if not record.is_published: | |||
There was a problem hiding this comment.
This is the fix. When interacting with the site in the browser it fixes the issue.
fenekku
force-pushed
the
app_rdm_2197_fix_new_version_bug
branch
4 times, most recently
from
8eb1259 to
1eeef7d
Compare
fenekku
changed the title
|
@Samk13 can you give this a review too since it addresses a problem you had too? |
fenekku
force-pushed
the
app_rdm_2197_fix_new_version_bug
branch
3 times, most recently
from
85044a4 to
68529b2
Compare
| # Classic draft of published record having been cleaned up scenario | ||
| record_1 = Record.publish(Draft.create({})) | ||
| db.session.commit() | ||
| Draft.cleanup_drafts(timedelta(0)) |
There was a problem hiding this comment.
The most typical case (but relies on cleanup_drafts).
| parent = ParentRecord.create({}) | ||
| record_2 = Record.create({}, parent=parent) | ||
| record_2.register() |
There was a problem hiding this comment.
The wider case that covers the above if cleanup_drafts works correctly and covers other scenarios e.g. direct forced deletion etc . In other words, anything that gets us to a published record without a draft.
|
@fenekku It's awesome that you've resolved this tricky issue! |
ntarocco
force-pushed
the
app_rdm_2197_fix_new_version_bug
branch
from
68529b2 to
40d9718
Compare
closes inveniosoftware/invenio-app-rdm#2197
Although small, the fix has a larger context to understand. When a record is published, its draft is soft-deleted in the db until
cleanup_draftshard deletes it. When that happens, the published record has no associated draft anymore, soeditwill create a completely new draft for it. However, that record may have a new version already (this is a fairly common occurrence in a running instance), so the new draft must not overwrite the reference to thenext_draft_idstored in the parent. Becausenext_draft_idis used both to refer to a record's next version and a never published before draft's own id, we get interference. By checking if we are in the case of a record that has been published (only records that can have a new version) versus a record that has not, we can appropriately set thatnext_draft_id.Another challenge with this PR was testing it. When trying to test this at the service level, we encounter OpenSearch's delete behavior (link to ElasticSearch but it applies to OpenSearch): the deleted document's
versionis kept around for 60s after deletion to overcome concurrency issues: Opensearch keeps a tombstone of deleted documents around and lets you reindex to a deleted document. So the version needs to stick around a little to make sure an update sent just before deletion was processed doesn't create a new record (updates can create new records in ES/OS).