manager: improe copy func #542
Conversation
kpsherva
force-pushed
the
manager-copy-many
branch
from
b962ceb to
8155b55
Compare
zzacharo
force-pushed
the
manager-copy-many
branch
2 times, most recently
from
a1f9de6 to
dc64686
Compare
| @@ -66,13 +66,14 @@ def get_by_key(cls, record_id, key): | |||
| @classmethod | |||
| def list_by_record(cls, record_id, with_deleted=False): | |||
| """List all record files by record ID.""" | |||
| query = cls.model_cls.query.filter(cls.model_cls.record_id == record_id) | |||
| with db.session.no_autoflush: | |||
There was a problem hiding this comment.
I am not sure about this change, could you explain how does it help?
There was a problem hiding this comment.
I think it was leftover from @jrcastro2 but in principle this is the fix we apply in other places instructing sqlalchemy not to trigger UPDATE queries when fetching from db. Is that the question?
There was a problem hiding this comment.
It helps very little, concretely it prevents 2 more queries to the DB every time this is called, the no_autoflush is useful when we are only querying, without executing any modifications on the data. Otherwise it can lead to inconsistent data. We can remove it if it's too complicated as the real gains are very little in this case.
invenio_records_resources/records/systemfields/files/manager.py
Outdated
Show resolved
Hide resolved
| self._entries = {} | ||
| for rf in self.file_cls.list_by_record(self.record.id): | ||
| self._entries[rf.key] = rf | ||
| else: |
There was a problem hiding this comment.
could we split this if statement to separate two methods instead? we know that the above block is used only while editing already published record, so we can use it directly
There was a problem hiding this comment.
I agree, but the point was not to change the existing chain of calls i.e draft_files.copy(record_files). I can do it if we align with @slint as we had discussed IRL and we said that this could be done later.
zzacharo
force-pushed
the
manager-copy-many
branch
from
dc64686 to
7ca0cb8
Compare
jrcastro2
force-pushed
the
manager-copy-many
branch
from
7ca0cb8 to
b075366
Compare
* closes zenodo/rdm-project#508 Co-authored-by: Javier Romero Castro <[email protected]> Co-authored-by: Karolina Przerwa <[email protected]>
jrcastro2
force-pushed
the
manager-copy-many
branch
from
b075366 to
1f52050
Compare
| @@ -54,7 +54,7 @@ tests = | |||
| invenio-db[postgresql,mysql,versioning]>=1.0.14,<2.0.0 | |||
| pytest-invenio>=2.1.0,<3.0.0 | |||
| pytest-mock>=1.6.0 | |||
| sphinx>=4.2.0,<5 | |||
| sphinx>=5,<6 | |||
There was a problem hiding this comment.
I had to update this in order for the tests to pass, the error was caused by one of the extensions
Apparently it was introduced some time ago but there was a release recently that maybe it's being picked up now, see here changelog.
The other alternative is to pin the extensions to <1.0.5
requires https://github.com/inveniosoftware/invenio-files-rest/pull/299/files