Repository navigation
ProjectMergeRequest.save() discards _optional_get_attrs #2007
Description
Description of the problem, including code/CLI snippet
When we request the MR object we can specify extra attributes to be returned to us. Once instantiated if we update part of the object and .save() it, our object loses these attributes.
Expected Behavior
The optional key:value in the MR attribute data should be retained
Actual Behavior
The keys and values for these particular attributes are lost
Specifications
- python-gitlab version:
- 3.4.0
- API version you are using (v3/v4):
- v4
- Gitlab server version (or gitlab.com):
- 13.12.10-ee
Here is a patch to retain these attributes across the .save() method. I'm working on a PR for this but tox is blowing up.
diff --git a/gitlab/mixins.py b/gitlab/mixins.py
index 1a3ff4d..1af1b2e 100644
--- a/gitlab/mixins.py
+++ b/gitlab/mixins.py
@@ -548,11 +548,29 @@ class SaveMixin(_RestObjectBase):
if not updated_data:
return None
+ # we need to save these attribute values across the .save() method
+ # or they'll be lost. the user didn't ask us to reload the MR object
+ # so be minimal in our actions
+ save_across_update = {}
+ for attr in self.manager._optional_get_attrs:
+ find_attr = attr
+ if find_attr.startswith("include_"):
+ find_attr = find_attr[8:]
+ if find_attr in self._attrs:
+ save_across_update[find_attr] = self._attrs[find_attr]
+
# call the manager
obj_id = self.encoded_id
if TYPE_CHECKING:
assert isinstance(self.manager, UpdateMixin)
server_data = self.manager.update(obj_id, updated_data, **kwargs)
+
+ # restore the optional requested attributes lost when we did the .save()
+ # the gitlab REST API doesn't offer a way to request these attributes
+ # during the POST operation thus we can't blindly overwrite our existing
+ # data with the new data
+ server_data.update(save_across_update)
+
self._update_attrs(server_data)
return server_data
Activity
nejch commentedon May 5, 2022
Thanks for the report @FirefighterBlu3. This limitation is described in https://python-gitlab.readthedocs.io/en/stable/api-usage.html#object-attributes.
I already had a generic fix for this for both
refresh()andsave()in #1443 but we decided against adding too much custom logic around the API.Does the workaround in the docs work for you?
I think we need to fix tox yes, just noticed recently. I think you'll need to run environments individually as one of the builds leaves artifacts that trip up one of the linters I think.
Edit: also keep in mind that
_optional_get_attrsare not really used by the API. This is used only by the CLI (confusing, I know - we need to untangle this). This is mostly about attributes returned by the initialget()which can have more details when passing certain parameters.JohnVillalovos commentedon May 5, 2022
I have submitted this PR: #2008
FirefighterBlu3 commentedon May 5, 2022
@nejch howdy, a pleasure. Thank you for the note pointing to the doc. Refetching with
.get()does work but it seems a bit heavyweight. I have a project that is going to be doing a lot of API back and forth and reloading objects is going to draw a load that I don't wish to happen. I would like the python wrapper to persist this data rather than make roughly 2x the necessary HTTP requests or maintain object state outside of the object. This becomes its own hairy nightmare ensuring secondary object data is reliably transported around, through whatever trenches are encountered.I've read through a number of the referenced issues just now. Yes, there are two sides to this coin regarding staying lightweight vs. doing the unexpected. I think it's harmful to do the unexpected and silently lose data even if it's documented. Don't do the unexpected is a fairly cardinal rule. We allow other user friendly settings to be defined once and we store them (pagination page size). We ought to be helpful here too.
Maybe instead of addressing the problem with potentially hairy logic as above, the
RESTObject._update_attrs()can be changed from:to
Although this will be insufficient if one of our desired optional attributes is mutated. In the end, the better solution is likely making optional attrs more persistent and getting the GitLab API to handle these in relevant REST calls.
FirefighterBlu3 commentedon May 5, 2022
I encountered this because I needed to figure out how to determine that a rebase is likely needed (and subsequent rebase_in_progress status) with diverging_commits_count as the base API doesn't initially expose the information or appropriate messages in the MR object as is available in the web UI
nejch commentedon May 5, 2022
This is essentially what the original PR in #1443 does, behind a flag for methods that manipulate the original object,
save()/refresh():) I understand the concern. Maybe we can revisit this topic if it's popular.nejch commentedon Sep 24, 2022
Just to follow up here, there are different scenarios:
Sometimes we don't want to persist attributes because they are stale compared to the real state on the server after certain API calls.
Sometimes we want to persist attributes as it's confusing UX that less is available on save/refresh.
Sometimes we have to persist attributes, or the object is completely broken (no ID available) after calling certain API calls
So I guess we will need to have some option here and document how to use it. I can't imagine going through each endpoint and handling when to persist things or not (e.g. when the server does not return all the attributes). But I'm also open to better ideas here :)
One previous attempt was #1443 though that would need updating a bit.