Repository navigation
Conversation
096d50b to
35fc996
Compare
| """ | ||
| draft_cls = current_rdm_records_service.draft_cls | ||
| return list( | ||
| draft_cls.get_records_by_parent( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
cool catch - fixed it. Could you recheck the logic in case I misunderstood something?
|
|
||
| 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": |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
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
61501a2 to
4194825
Compare
| the LHC' | ||
| props: | ||
| link: http://totem.web.cern.ch/Totem/ | ||
| - id: H1 |
There was a problem hiding this comment.
Should be in a separate PR
| 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) |
There was a problem hiding this comment.
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?
41b1a68 to
51fb881
Compare
This branch has not been deployed
3 participants
*closes #1016