harvester: fail on existing draft by kpsherva · Pull Request #1019 · CERNDocumentServer/cds-rdm

kpsherva · 2026-10-06T18:20:38Z

*closes #1016

TahaKhan998 · 2026-10-09T11:55:42Z

+        """
+        draft_cls = current_rdm_records_service.draft_cls
+        return list(
+            draft_cls.get_records_by_parent(


edit drafts have the same cdsrdm id as the published version. this line loads every draft under the parent so it loads edit drafts of v1, v2, whatever, plus new-version drafts.
and then later at line 123 we only check if the draft id matches the record we're updating (usually latest). so if theres an edit draft on v1 and the harvester is updating v2 (no draft on v2), we still pick up the v1 draft here, then at line 123 it doesnt match v2 so we treat it like a new-version draft, compare it to v2, it looks changed, and we block the harvest even though nobody was editing v2.

we should compare the edit draft to the published version when we are dealing with edit drafts, and only check if there are changes for new version drafts.

cool catch - fixed it. Could you recheck the logic in case I misunderstood something?

TahaKhan998 · 2026-10-09T11:58:14Z

+
+    def check(self, stream_entry, *, record=None, record_pid=None, matcher=None):
+        """Return an error if the record's files are restricted."""
+        if record.get("access", {}).get("files") != "restricted":


if there are metadata changes on a record with restricted files, we still stop the harvester from updating even though the restricted files didnt have any changes and werent going to be updated.

is that intentional (restricted files = dont touch the record at all), or should metadata still sync?

this is intentional - I don't want to cause the metadata to be out of sync in regard to the files - since the most important part of the record is not synced - let's fail all of it.

TahaKhan998 · 2026-10-09T12:05:37Z

+    def _fetch_conference(self, url, logger):
+        """Fetch the INSPIRE conference metadata, ``None`` on failure."""
+        try:
+            response = requests.get(url, timeout=self.timeout)
+            response.raise_for_status()
+            return response.json().get("metadata", {})
+        except (requests.RequestException, ValueError) as e:
+            logger.warning(
+                f"Could not fetch INSPIRE conference. | details: url={url}, error={e}"
+            )
+            return None


Suggested change

def _fetch_conference(self, url, logger):

"""Fetch the INSPIRE conference metadata, ``None`` on failure."""

try:

response = requests.get(url, timeout=self.timeout)

response.raise_for_status()

return response.json().get("metadata", {})

except (requests.RequestException, ValueError) as e:

logger.warning(

f"Could not fetch INSPIRE conference. | details: url={url}, error={e}"

)

return None

_conference_cache = {}

def _fetch_conference(self, url, logger):

"""Fetch the INSPIRE conference metadata, ``None`` on failure."""

if url in ConferenceMapper._conference_cache:

return ConferenceMapper._conference_cache[url]

try:

response = requests.get(url, timeout=self.timeout)

response.raise_for_status()

metadata = response.json().get("metadata", {})

except (requests.RequestException, ValueError) as e:

logger.warning(

f"Could not fetch INSPIRE conference. | details: url={url}, error={e}"

)

metadata = None

ConferenceMapper._conference_cache[url] = metadata

return metadata

just a suggestion: every time we map a paper we fetch the conference from inspire. seen only stops us refetching inside the same paper, so if we harvest a bunch of papers from the same conference we call the same url again and again. maybe we just cache by url so we only fetch each conference once in a harvest

I plan to ask INSPIRE team to include the conference information inside the JSON format they are generating for us so this code will not be necessary in the future, I would keep it simple. But I think the idea in general is nice

sakshamarora1 · 2026-10-14T14:50:02Z

      the LHC'
  props:
    link: http://totem.web.cern.ch/Totem/
- id: H1


Should be in a separate PR

sakshamarora1 · 2026-10-14T15:18:42Z

+        for draft in draft_cls.get_records_by_parent(
+            record._record.parent, with_deleted=False
+        ):
+            draft_id = str(draft.pid.pid_value)
+            is_new_version = not self._is_published(record_cls, draft_id)


get_records_by_parent already returns the draft here, so I don't get why we need another DB query via _is_published to get the same PID for all, seems redundant. We can just check if not draft.is_published?

zzacharo · 2026-10-29T08:09:41Z

+        draft_dict = dict(draft)
+        record_dict = dict(record)
+        is_edit_draft = str(draft.pid.pid_value) == str(record.pid.pid_value)
+        # A new version gets its own pids, so only edit drafts are comparable.


How this comment is associated with the line below?

zzacharo · 2026-10-29T08:10:47Z

+        record_dict = dict(record)
+        is_edit_draft = str(draft.pid.pid_value) == str(record.pid.pid_value)
+        # A new version gets its own pids, so only edit drafts are comparable.
+        if is_edit_draft and draft_dict.get("pids", {}).get(


nit: for readability maybe save these in variables and compare e.g draft_doi != record_doi

zzacharo · 2026-10-29T08:14:19Z

+        record_files = self._file_signatures(record)
+        if is_edit_draft:
+            return draft_files != record_files
+        return not draft_files <= record_files


I guess here is the new version draft and we are checking if in the new version there are less files than in the published one? Can you explain the reasoning as it is not clear to me?

The writer uses it to decide whether a user has changed a draft, which blocks the harvest. A new-version draft is created by copying the published record, but its files may not be copied over yet. So an empty
draft, or one with fewer files, is not a user (deliberate) change.

Edit draft: it should match the record exactly, so the code compares draft_files != record_files.

New-version draft: the code returns not draft_files <= record_files, which is true only if the draft has a file the record lacks, or a file with a different checksum.

Fewer files in the new version: the code deliberately does not count this as a change.

So user who deliberately deleted a file from a new version draft would therefore not be detected, but it is a trade-off that this draft will be discarded as probably it should.... since the harvester brings "the latest" state presumably

zzacharo · 2026-10-29T08:18:20Z

+    assert service.read_draft(system_identity, record["id"])
+
+
+def test_update_discards_unchanged_new_version_draft(


Can you also check the branch that a new_version exists but it has changes?

zzacharo · 2026-10-29T08:19:07Z


+
+
+def test_update_fails_when_draft_exists(


similarly here: if there is an edited draft but no changes have been applied, does the harvester complete the update?

zzacharo · 2026-10-29T08:21:21Z

+            content=content,
+        )
+        record = current_rdm_records_service.publish(system_identity, draft.id)
+        add_legacy_recid(add_pid, record, "2882312")


the test claims multiple versions and the latest has restricted files. Here, it seems we create only one version? If so, can you replicate actually multiple ones?

you are right, the test only created one version, so I changed it to create two: v1 public, and v2 as a report with restricted files.
I removed the duplicate check from _resource_type_versioning because it was not reachable, already handled by validation

kpsherva force-pushed the harvester-fixed-prod branch from 096d50b to 35fc996 Compare October 8, 2026 18:11

TahaKhan998 reviewed Oct 9, 2026

View reviewed changes

kpsherva force-pushed the harvester-fixed-prod branch 2 times, most recently from 61501a2 to 4194825 Compare October 12, 2026 14:11

sakshamarora1 reviewed Oct 14, 2026

View reviewed changes

kpsherva added 6 commits October 15, 2026 19:04

add(harvester): fail if there is already existing draft on update

cee1296

change(harvester): check if draft has any data changed before update

1f62828

add(harvester): conference mapping

83ef96d

add(harvester): fail on restricted record update attempt

72b70ba

fix(harvester): compare metadata on update

0612aca

fix(harvester): consider unregistered pids on publish check

51fb881

kpsherva force-pushed the harvester-fixed-prod branch from 41b1a68 to 51fb881 Compare October 15, 2026 17:04

zzacharo reviewed Oct 29, 2026

View reviewed changes

fix(harvester): check if draft is published

6cf8e97

kpsherva force-pushed the harvester-fixed-prod branch from 6ee7acf to 6cf8e97 Compare October 29, 2026 12:06

zzacharo approved these changes Nov 2, 2026

View reviewed changes

sakshamarora1 approved these changes Nov 3, 2026

View reviewed changes

kpsherva merged commit db785aa into CERNDocumentServer:master Nov 4, 2026
3 checks passed

		assert service.read_draft(system_identity, record["id"])


		def test_update_discards_unchanged_new_version_draft(

Conversation

kpsherva commented Oct 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpsherva Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpsherva Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kpsherva Oct 12, 2026 •

edited

Loading

kpsherva Oct 29, 2026 •

edited

Loading