tests(e2e2): cover the "..." RRset endpoint and bulk request ordering by acoseac · Pull Request #1232 · desec-io/desec-stack

acoseac · 2026-08-15T20:57:40Z

Closes #171
Closes #226

Two commits, as discussed in review.

1. Drop the undocumented `{subname}@` URL form

The RRset detail endpoint accepted rrsets/{subname}@/{type}/ as a side effect of the route that makes the zone apex addressable as rrsets/@/{type}/. Only the apex spelling is documented; for non-empty subnames the documented form is rrsets/{subname}.../{type}/, and nothing outside the test helpers used it.

The route is removed and the helpers in base.py now reverse the ... form, which the suite already used elsewhere. The apex @ form is unaffected. rrsets/{subname}@/{type}/ falls through to the generic route, where {subname}@ is not a valid subname, and returns 404.

2. Tests

The ... form was covered by no test at all, since base.py reversed the @ form throughout.

desecapi/tests/test_rrsets.py covers the URL spellings over apex, plain, wildcard, dotted and underscore subnames, and that the removed form now returns 404
test/e2e2 keeps only what needs the full stack: that these characters survive the HTTP layer in front of the API, that the apex is not addressable by leaving the subname empty, and that a write made through such a URL reaches the DNS
bulk ordering: the AAAA/CNAME replacement from API: Cannot Replace AAAA with CNAME in Single Request #220 in both payload orders, for PATCH and PUT, in both directions, against the real pdns
the positional error array turned out to be covered already by test_bulk_patch_missing_invalid_fields_1, so only the missing atomicity check is added next to it

put() is added to the e2e API client, which had get/post/patch/delete but no PUT.

Verification

Both suites run against a full local stack, rebased on current main:

API test suite: passes
e2e2 suite: 338 passed, 2 skipped

peterthomassen

Thank you for engaging with our backlog! <3

peterthomassen · 2026-08-18T18:12:33Z

+@pytest.mark.parametrize("position", [0, 1, 2])
+def test_errors_are_reported_in_request_order(api_user_domain: DeSECAPIV1Client, position: int):
+    # Errors are returned per part of the bulk request, in the order given, with
+    # an empty object for the parts that passed.
+    api = api_user_domain
+    payload = [
+        {'subname': f'valid{i}', 'type': 'A', 'ttl': TTL, 'records': ['1.2.3.4']}
+        for i in range(3)
+    ]
+    payload[position] = {'subname': 'invalid', 'type': 'A', 'ttl': -1, 'records': ['1.2.3.4']}
+
+    response = api.patch(f"/domains/{api.domain}/rrsets/", data=payload)
+    assert response.status_code == 400
+    errors = response.json()
+    assert [bool(error) for error in errors] == [i == position for i in range(3)]
+    assert 'ttl' in errors[position]
+
+    # bulk requests are atomic, so the valid parts were not applied either
+    assert api.get(f"/domains/{api.domain}/rrsets/?type=A").json() == []


This test hits the API only. Does it needs to live as an e2e test?

That one is pure API. Turns out test_bulk_patch_missing_invalid_fields_1 already covers the positional errors, so I'll drop it here and add just the missing atomicity check to the unit suite.

peterthomassen · 2026-08-18T18:14:54Z

+# https://desec.readthedocs.io/en/latest/dns/rrsets.html#accessing-the-zone-apex
+#
+#     {subname}/{type}/     plain
+#     {subname}@/{type}/    '@' terminates the subname


This is actually only documented for the empty subname: rrsets/@/<type>/. For non-empty subname, rrsets/<subname>.../<type>/ is document, but not rrsets/<subname>@/<type>/ (although it is used in testing).

To minimize unneeded/undocumented or testing-only code, perhaps we should remove the <subname>@ form (switching tests to ... form). What do you think?

I personally also feel that URLs like domains/example.com/rrsets/sub@/A/ are less intuitive than domains/example.com/rrsets/sub.../A/ (albeit 2 characters shorter), but that's subjective.

Thanks for the review!

Agreed, let's drop it.

Removing the route itself and switching base.py to ... is an API change, so probably its own PR. Only caveat: it's technically breaking for anyone who found it, undocumented or not.

In the last 31 days, we had 1 user doing a GET request like that. I've informed them and I think it's fine to drop it.

If you don't mind, I'd prefer this PR to address the full topic, that is, taking a fresh look at the situation, which leads to dropping the one route and adding tests for the other. I agree it should be different commits. How does that sounds to you?

Sounds good, will do it here in two commits: one dropping the route and switching the test helpers to the ... form, one for the tests.

peterthomassen · 2026-08-18T18:20:05Z

+@pytest.mark.parametrize("subname", SUBNAMES)
+def test_retrieve(api_user_domain: DeSECAPIV1Client, subname: str):
+    api = api_user_domain
+    assert api.rr_set_create(api.domain, 'A', ['1.2.3.4'], subname=subname, ttl=TTL).status_code == 201
+
+    responses = [api.get(url) for url in rrset_urls(api.domain, subname, 'A')]
+    for response in responses:
+        assert response.status_code == 200
+        assert response.json()['subname'] == subname
+        assert response.json()['records'] == ['1.2.3.4']
+    assert all(response.json() == responses[0].json() for response in responses)
+
+
+@pytest.mark.parametrize("subname", SUBNAMES)
+def test_patch(api_user_domain: DeSECAPIV1Client, subname: str):
+    api = api_user_domain
+    assert api.rr_set_create(api.domain, 'A', ['1.2.3.4'], subname=subname, ttl=TTL).status_code == 201
+
+    # every URL form can be used for modification, and they all address the same RRset
+    for i, url in enumerate(rrset_urls(api.domain, subname, 'A'), start=1):
+        response = api.patch(url, data={'ttl': TTL + i})
+        assert response.status_code == 200
+        assert response.json()['subname'] == subname
+        assert response.json()['ttl'] == TTL + i
+        assert api.get(rrset_urls(api.domain, subname, 'A')[0]).json()['ttl'] == TTL + i
+
+
+@pytest.mark.parametrize("subname", SUBNAMES)
+def test_put(api_user_domain: DeSECAPIV1Client, subname: str):
+    api = api_user_domain
+    assert api.rr_set_create(api.domain, 'A', ['1.2.3.4'], subname=subname, ttl=TTL).status_code == 201
+
+    for i, url in enumerate(rrset_urls(api.domain, subname, 'A')):
+        records = [f'10.0.0.{i}']
+        response = api.put(url, data={'subname': subname, 'type': 'A', 'ttl': TTL, 'records': records})
+        assert response.status_code == 200
+        assert response.json()['records'] == records
+        assert api.get(rrset_urls(api.domain, subname, 'A')[0]).json()['records'] == records
+
+
+@pytest.mark.parametrize("subname", SUBNAMES)
+def test_delete(api_user_domain: DeSECAPIV1Client, subname: str):
+    api = api_user_domain
+
+    for url in rrset_urls(api.domain, subname, 'A'):
+        assert api.rr_set_create(api.domain, 'A', ['1.2.3.4'], subname=subname, ttl=TTL).status_code == 201
+        assert api.delete(url).status_code == 204
+        assert api.get(url).status_code == 404
+        assert api.delete(url).status_code == 204  # deletion is idempotent
+
+
+def test_apex_not_addressable_with_empty_subname(api_user_domain: DeSECAPIV1Client):
+    # This is the reason the '@' and '...' forms exist: the double slash does not
+    # survive URL normalization, so the apex cannot be addressed by omitting the
+    # subname.
+    api = api_user_domain
+    assert api.rr_set_create(api.domain, 'A', ['1.2.3.4'], ttl=TTL).status_code == 201
+
+    assert api.get(f"/domains/{api.domain}/rrsets//A/").status_code == 404
+    assert api.get(f"/domains/{api.domain}/rrsets/.../A/").status_code == 200


These tests do not use the DNS; they hit the API only. Do they need to be e2e tests?

Proposal is to keep a thin e2e test for the URL shapes and move the exhaustive matrix to unit tests.

sure, doesn't hurt!

The RRset detail endpoint accepted `rrsets/{subname}@/{type}/` as a side effect of the route that makes the zone apex addressable as `rrsets/@/{type}/`. Only the apex spelling is documented; for non-empty subnames the documented form is `rrsets/{subname}.../{type}/`. Nothing outside the test helpers used it. Drop the route and point the test helpers at the `...` form, which the test suite already uses elsewhere. The apex `@` form is unaffected; `rrsets/{subname}@/{type}/` now falls through to the generic route, where `{subname}@` is not a valid subname, and returns 404.

The `...` URL form was covered by no test at all, since the helpers in base.py reversed the `@` form throughout. Cover the remaining spellings in the API test suite, where the matrix over subnames belongs, and keep in e2e only what needs the full stack: that the unusual characters survive the HTTP layer in front of the API, that the apex is not addressable by leaving the subname empty, and that a write made through such a URL reaches the DNS. For bulk requests, the outcome must not depend on the order in which the RRsets are given. pdns refuses to add a CNAME while a conflicting RRset is still present, so the API applies deletions before additions (desec-io#220, PowerDNS/pdns#7501); the existing unit test covers that against a mocked pdns and for one payload order only. Exercise both orders, for PATCH and PUT, in both directions, against the real thing. The positional error array is already covered by test_bulk_patch_missing_invalid_fields_1, so only the missing atomicity check is added alongside it. Closes desec-io#171 Closes desec-io#226

acoseac · 2026-08-19T08:13:05Z

Pushed, rebased on current main, two commits as discussed.

One thing worth flagging: the positional error array is already covered by test_bulk_patch_missing_invalid_fields_1, so I dropped that test rather than porting it, and added only the missing atomicity check next to it.

Ran both suites against a full local stack: API tests pass, e2e2 338 passed and 2 skipped.

acoseac mentioned this pull request Aug 15, 2026

docker compose build fails on a first build: api and celery-email export the same image tag #1233

Open

peterthomassen reviewed Aug 18, 2026

View reviewed changes

acoseac added 2 commits August 19, 2026 10:12

acoseac force-pushed the tests/e2e2-rrset-url-and-bulk-order branch from 6306465 to b2d04c7 Compare August 19, 2026 08:12

Navigation Menu

Uh oh!

Uh oh!

tests(e2e2): cover the "..." RRset endpoint and bulk request ordering - #1232

tests(e2e2): cover the "..." RRset endpoint and bulk request ordering#1232
acoseac wants to merge 2 commits into
desec-io:mainfrom
acoseac:tests/e2e2-rrset-url-and-bulk-order

acoseac commented Aug 15, 2026 •

edited

Loading

Uh oh!

peterthomassen left a comment

Uh oh!

peterthomassen Aug 18, 2026

Uh oh!

acoseac Aug 18, 2026

Uh oh!

peterthomassen Aug 18, 2026

Uh oh!

acoseac Aug 18, 2026

Uh oh!

peterthomassen Aug 18, 2026

Uh oh!

acoseac Aug 19, 2026

Uh oh!

peterthomassen Aug 18, 2026

Uh oh!

acoseac Aug 18, 2026

Uh oh!

peterthomassen Aug 18, 2026

Uh oh!

acoseac commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

acoseac commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Drop the undocumented {subname}@ URL form

2. Tests

Verification

Uh oh!

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

acoseac commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

acoseac commented Aug 15, 2026 •

edited

Loading

1. Drop the undocumented `{subname}@` URL form