Update batch API endpoint, openapi documentation and add batch test #567

xkello · 2026-01-26T15:08:34Z

Work on ticket: https://github.com/MerginMaps/server-private/issues/3181
-from parent: https://github.com/MerginMaps/product/issues/86

coveralls · 2026-01-26T15:17:29Z

Pull Request Test Coverage Report for Build 21519816080

Details

100 of 106 (94.34%) changed or added relevant lines in 5 files are covered.
21 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.04%) to 94.367%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
server/mergin/sync/public_api_v2_controller.py	12	14	85.71%
server/mergin/sync/permissions.py	17	21	80.95%

Files with Coverage Reduction	New Missed Lines	%
server/mergin/tests/test_utils.py	1	99.01%
server/mergin/utils.py	20	67.5%

Totals
Change from base Build 21353134513:	-0.04%
Covered Lines:	8192
Relevant Lines:	8681

💛 - Coveralls

MarcelGeo

Nice 🎉

we need to double check if 404 is ok if you have empty projects list or return error: 404
check def get_projects_by_uuids(uuids) method in project_api_controller.py if everything with permissions is ok here

server/mergin/sync/public_api_v2_controller.py

server/mergin/tests/test_public_api_v2.py

server/mergin/sync/permissions.py

harminius

Very good job!

I think two things need to be discussed (to fulfill clients' requirements):
1, Shall all (valid) uuids coming from the request be part of the response or can we skip them as suggested here?
2, Do we need to differentiate between 404 and 403 here with the expose flag? I tend to make things easier by keeping only 404 error.

server/mergin/sync/public_api_v2.yaml

server/mergin/sync/public_api_v2_controller.py

server/mergin/sync/public_api_v2.yaml

server/mergin/sync/public_api_v2_controller.py

server/mergin/sync/permissions.py

server/mergin/tests/test_public_api_v2.py

server/mergin/sync/public_api_v2.yaml

harminius · 2026-01-27T09:30:38Z

@xkello Although your approach returns a correct response, I think we need to slightly change the approach.
You can keep the require_project_by_many_uuids() function, but when making the response, I would compare the incoming uuids with ones we have found projects for. All the rest should get the error code.
That's how I understand this requirement: https://github.com/MerginMaps/server-private/issues/3155#issuecomment-3804048237

server/mergin/sync/permissions.py

…reuseability in future

harminius

Great job, we're almost there! 👍

I have only one major comment.
I have doubts about the usefulness of require_project_by_many_uuids(). At least it should return a list of projects, and the rest (by_id dict creation) should be done in the controller. However, since this function needs to list even removed projects, it doesn't seem reusable.
The idea of the help function is that it can be used in a different use case, while this function is designed for this specific usecase.

harminius · 2026-01-30T10:05:27Z

server/mergin/sync/permissions.py

 def require_project_by_many_uuids(
-    uuids: list[str], permission: ProjectPermissions, scheduled=False, expose=True
+    uuids: list[str], permission: ProjectPermissions
 ) -> list[dict]:


would be nice if it returned list[Project]

Cant be done I believe since we are returning Project objects as well as per UUID errors. The Logic would have to be split somehow

Dict juggling on lines 272 to 296 can nicely be done in the controller.
Now I see it can also filter out those removed projects which is desirable.
Just a bit of code moving, no logic change to make the output of this function generic and reusable.

harminius · 2026-01-30T10:11:06Z

server/mergin/sync/permissions.py

+    projects = (
+        Project.query
+        .filter(Project.id.in_(valid_uuids))
+        .filter(Project.storage_params.isnot(None))


i think also these should be included in the response with 404 error

This is already handled at line 277-284. When uuid is not present in project lists it returns 404 automatically regardless of the reason. So in a way the information why was 404 returned is lost but I dont see a reason to store that info since we dont use it - just a side note though

I see. I'd also filter out Project.removed_at.is_(None) then.

harminius · 2026-01-30T10:17:00Z

server/mergin/tests/test_permissions.py

+    # First user with access to both projects
+    login(client, DEFAULT_USER[0], DEFAULT_USER[1])
+
+    with client:


Hmm.. this is to pass current user?

Yes and no, the line under is also needed. without these 2 test failure was returning error where user was none. I believe it has something to do with active request context.

harminius · 2026-01-30T10:27:03Z

server/mergin/tests/test_public_api_v2.py


-    # must be logged in (matches suite behavior)
-    login(client, DEFAULT_USER[0], DEFAULT_USER[1])
+    #login(client, DEFAULT_USER[0], DEFAULT_USER[1]) TODO: why needed?


harminius · 2026-01-30T10:44:49Z

server/mergin/sync/public_api_v2.yaml

+                  type: array
+                  description: List of project UUIDs to fetch
+                  items:
+                    $ref: "#/components/parameters/ProjectId"


It's probably due to my previous comment - sorry for that, but here is the path parameter referenced
A clean solution would be to decouple the definition from the location - to move the definition under the Schema component and put the reference in the Parameters components.
Then you can safely reference the schema here in the request body definition

components: parameters: ProjectIdParam: name: id in: path required: true schema: $ref: '#/components/schemas/ProjectIdSchema'

harminius · 2026-01-30T10:48:54Z

server/mergin/sync/public_api_v2.yaml

+      type: object
+      properties:
+        id:
+          $ref: "#/components/parameters/ProjectId"


the same previous comment - reference a ProjectId schema here, not parameter

Update batch API endpoint, openapi documentation and add batch test

254a9b8

MarcelGeo requested review from harminius and varmar05 January 27, 2026 07:38

MarcelGeo requested changes Jan 27, 2026

View reviewed changes

harminius requested changes Jan 27, 2026

View reviewed changes

harminius reviewed Jan 29, 2026

View reviewed changes

server/mergin/sync/permissions.py Outdated Show resolved Hide resolved

harminius reviewed Jan 29, 2026

View reviewed changes

server/mergin/sync/permissions.py Outdated Show resolved Hide resolved

harminius reviewed Jan 29, 2026

View reviewed changes

server/mergin/sync/permissions.py Outdated Show resolved Hide resolved

harminius reviewed Jan 29, 2026

View reviewed changes

server/mergin/sync/permissions.py Outdated Show resolved Hide resolved

xkello added 2 commits January 30, 2026 10:50

Make changes to code to better represent specs, split test logic for …

235f1b1

…reuseability in future

add name to CLA-signed-list

aa1eb6d

harminius requested changes Jan 30, 2026

View reviewed changes

xkello added 2 commits January 30, 2026 13:11

Add test fix where user was getting Project object instead of an error

d44dfee

Fix 2 on test for permissions function

1914dc3

Update batch API endpoint, openapi documentation and add batch test #567

Are you sure you want to change the base?

Update batch API endpoint, openapi documentation and add batch test #567

Uh oh!

Conversation

xkello commented Jan 26, 2026

Uh oh!

coveralls commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21519816080

Details

💛 - Coveralls

Uh oh!

MarcelGeo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harminius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harminius commented Jan 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harminius 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 26, 2026 •

edited

Loading