Implement a way to return the sequential_number after create by luisa-beerboom · Pull Request #3326 · OpenSlides/openslides-backend

luisa-beerboom · 2026-02-10T16:00:50Z

hjanott

More questions than a review. Let's talk tomorrow.

hjanott · 2026-02-12T16:56:59Z

openslides_backend/services/database/database_writer.py

        self.print_stats()
        self.print_summary()
-        return sorted(modified_models)
+        return modified_models


This sorting is obsolete?

modified_models was a set of fqids before and this sorting transformed it into a sorted list, however it is now a fqid to model data dict and thus sorting it would turn it into a list of sorted fqid-model data tuples. As far as I could see, all places calling this ought to be able to handle a dict just as well as a sorted list, but I could be wrong. It's a bit hard to see if this breaks anything when this is still based on a version of reldb with many regularly broken tests.

In the end this was a temporary fix that was resolved cleanly now.

hjanott · 2026-02-12T16:57:50Z

openslides_backend/action/action_handler.py

+                    if element:
+                        self.update_action_data_with_write_results(element, results)
+            elif isinstance(data, dict):
+                edit_fn = data.pop(META_POST_EDIT_FN_KEY, None)


I still need to think if I like this function thing.

You and me both, however as far as I can see, the alternatives would be to
A. Move the write calls into the action class. I think that'd be quite a bit of work, and, since there's so many different kinds of action classes and restructuring something so central in that invasive manner is a bit dangerous, risky.
B. Use a meta field containing the information necessary to find the fqid. That way it could filter for that instead and then just use a generalized update function. That would make individual solutions like the nested post-editing in the motion_forward impossible though.

I think A would actually be the optimum. Then again, I don't think doing it this way is particularly unsafe or bad in terms of performance. The function field will be consistently filtered out. It won't break anything unless we actually add a field META_EDIT_FN_GHETZHSRHSRGG to a model (unlikely), and even in that case we can just change the constant to another string and it'll work again. Also the entire filtering through the object in search of a meta edit field only happens if a meta edit field was set anyway, so most actions won't even be affected.

If you prefer any of the other options, or you would rather have something completely different happen, feel free to tell me though, I'll work on that then.

hjanott · 2026-02-12T17:01:35Z

openslides_backend/action/actions/motion/base_create_forwarded.py

+    def post_edit_fn(self, data: dict, results: dict[str, dict[str, Any]]) -> None:
+        super().post_edit_fn(data, results)
+        dates = [*data.get("amendment_result_data", [])]
+        while len(dates):
+            date = dates.pop(0)
+            super().post_edit_fn(date, results)
+            dates.extend(date.get("amendment_result_data", []))


I guess this is some special casing, that was also broken?

See this actions create_action_result_element method and the create_forwarded test: The forwarding action creates a nested result element with the result elements of created amendments in an array of their lead motion. If I want the sequential_number for each of them in their part of the element, I'm going to need to parse through the stuff iteratively, when I don't have to do it for regular actions. That's why I overrode the edit function in this way.

This is the reason I stayed with the callback function variant of this instead of just adding a meta field.
I had some doubt if this was actually technically necessary since I'm not sure if the client even has a need for the amendment seq nums, however for the sake of consistency...

hjanott · 2026-02-12T17:02:31Z

openslides_backend/action/action.py

+    def get_post_edit_function(self) -> EditFunction | None:
+        """
+        Returns a function that allows post-editing the action result
+        with the return data from the database writer.
+        """
+        return None


What do you think? Could we handle the id in this base class in an analogue manner? Since in the future we don't want to use reserve_ids.

Do you mean the return id of the create actions?

Because as far as I could see it while looking at the write action, it already kind of read the id in that manner...

Idk. If the sub-actions (or actions in general) use the write function directly, maybe. But currently I think it's only used in the action_handler when reading out the write requests. That is too late to use the ids within the actions f.e. to write the new models id into foreign id fields of newly created sub-models or vice versa. However, if we moved the writing to the action class it may be possible, at least in some cases. I think that's a reldb2 issue though.

So we should first implement the direct writes and then we will change the system for ids and seq numbers.

luisa-beerboom · 2026-02-13T08:43:17Z

openslides_backend/services/database/database_writer.py

        self.print_stats()
        self.print_summary()
-        return sorted(modified_models)
+        return modified_models


modified_models was a set of fqids before and this sorting transformed it into a sorted list, however it is now a fqid to model data dict and thus sorting it would turn it into a list of sorted fqid-model data tuples. As far as I could see, all places calling this ought to be able to handle a dict just as well as a sorted list, but I could be wrong. It's a bit hard to see if this breaks anything when this is still based on a version of reldb with many regularly broken tests.

luisa-beerboom · 2026-02-13T08:49:55Z

openslides_backend/action/actions/motion/base_create_forwarded.py

+    def post_edit_fn(self, data: dict, results: dict[str, dict[str, Any]]) -> None:
+        super().post_edit_fn(data, results)
+        dates = [*data.get("amendment_result_data", [])]
+        while len(dates):
+            date = dates.pop(0)
+            super().post_edit_fn(date, results)
+            dates.extend(date.get("amendment_result_data", []))


See this actions create_action_result_element method and the create_forwarded test: The forwarding action creates a nested result element with the result elements of created amendments in an array of their lead motion. If I want the sequential_number for each of them in their part of the element, I'm going to need to parse through the stuff iteratively, when I don't have to do it for regular actions. That's why I overrode the edit function in this way.

This is the reason I stayed with the callback function variant of this instead of just adding a meta field.
I had some doubt if this was actually technically necessary since I'm not sure if the client even has a need for the amendment seq nums, however for the sake of consistency...

luisa-beerboom · 2026-02-13T09:02:07Z

openslides_backend/action/action.py

+    def get_post_edit_function(self) -> EditFunction | None:
+        """
+        Returns a function that allows post-editing the action result
+        with the return data from the database writer.
+        """
+        return None


Do you mean the return id of the create actions?

Because as far as I could see it while looking at the write action, it already kind of read the id in that manner...

Idk. If the sub-actions (or actions in general) use the write function directly, maybe. But currently I think it's only used in the action_handler when reading out the write requests. That is too late to use the ids within the actions f.e. to write the new models id into foreign id fields of newly created sub-models or vice versa. However, if we moved the writing to the action class it may be possible, at least in some cases. I think that's a reldb2 issue though.

luisa-beerboom · 2026-02-13T09:15:01Z

openslides_backend/action/action_handler.py

+                    if element:
+                        self.update_action_data_with_write_results(element, results)
+            elif isinstance(data, dict):
+                edit_fn = data.pop(META_POST_EDIT_FN_KEY, None)


You and me both, however as far as I can see, the alternatives would be to
A. Move the write calls into the action class. I think that'd be quite a bit of work, and, since there's so many different kinds of action classes and restructuring something so central in that invasive manner is a bit dangerous, risky.
B. Use a meta field containing the information necessary to find the fqid. That way it could filter for that instead and then just use a generalized update function. That would make individual solutions like the nested post-editing in the motion_forward impossible though.

I think A would actually be the optimum. Then again, I don't think doing it this way is particularly unsafe or bad in terms of performance. The function field will be consistently filtered out. It won't break anything unless we actually add a field META_EDIT_FN_GHETZHSRHSRGG to a model (unlikely), and even in that case we can just change the constant to another string and it'll work again. Also the entire filtering through the object in search of a meta edit field only happens if a meta edit field was set anyway, so most actions won't even be affected.

If you prefer any of the other options, or you would rather have something completely different happen, feel free to tell me though, I'll work on that then.

luisa-beerboom · 2026-02-13T09:19:20Z

tests/system/action/motion/test_create_forwarded.py

                "id": 18,
+                "sequential_number": 4,
                "non_forwarded_amendment_amount": 1,
                "amendment_result_data": [
                    {
                        "id": 19,
+                        "sequential_number": 1,
                        "non_forwarded_amendment_amount": 0,
                        "amendment_result_data": [],
                    },
                    {
                        "id": 20,
+                        "sequential_number": 3,
                        "non_forwarded_amendment_amount": 1,
                        "amendment_result_data": [
                            {
                                "id": 21,
+                                "sequential_number": 2,
                                "non_forwarded_amendment_amount": 0,
                                "amendment_result_data": [],


Btw. I find the seq num generation here confusing. I have no idea if there even is a way to change it though. Maybe a future issue?

…eturn-seq-nums

hjanott

Looks good now. Thank you for deleting those commented out lines.

hjanott · 2026-02-24T17:30:40Z

openslides_backend/action/action.py

+    def get_post_edit_function(self) -> EditFunction | None:
+        """
+        Returns a function that allows post-editing the action result
+        with the return data from the database writer.
+        """
+        return None


So we should first implement the direct writes and then we will change the system for ids and seq numbers.

Implement a way to return the sequential_number after create

7d25353

luisa-beerboom added this to the 4.3 milestone Feb 10, 2026

luisa-beerboom self-assigned this Feb 10, 2026

luisa-beerboom added the bug label Feb 10, 2026

Add mixin to other actions and test

7a7b3f4

luisa-beerboom requested a review from hjanott February 11, 2026 16:29

Improve functionality

7340449

luisa-beerboom marked this pull request as ready for review February 12, 2026 12:26

luisa-beerboom assigned hjanott and unassigned luisa-beerboom Feb 12, 2026

hjanott reviewed Feb 12, 2026

View reviewed changes

luisa-beerboom commented Feb 13, 2026

View reviewed changes

luisa-beerboom requested a review from hjanott February 13, 2026 09:21

hjanott deleted the branch OpenSlides:main February 20, 2026 18:40

hjanott closed this Feb 20, 2026

hjanott reopened this Feb 20, 2026

luisa-beerboom changed the base branch from feature/relational-db to main February 24, 2026 15:15

luisa-beerboom added 5 commits February 25, 2026 11:27

Merge remote-tracking branch 'upstream/main' into 3299-make-backend-r…

b9461b2

…eturn-seq-nums

Update meta

3c8571c

Merge remote-tracking branch 'upstream/main' into 3299-make-backend-r…

dc7f392

…eturn-seq-nums

Fix failing test

eccb47e

Remove commented out code from database writer

cbaf6fa

hjanott approved these changes Feb 25, 2026

View reviewed changes

Fix linting

f133134

luisa-beerboom merged commit a46899a into OpenSlides:main Feb 25, 2026
5 checks passed

Conversation

luisa-beerboom commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hjanott left a comment

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!

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!

hjanott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

luisa-beerboom commented Feb 10, 2026 •

edited

Loading