Repository navigation
[chores:fix] Fixed PKI API query count regression due to DRF 3.18 - #1503
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (10)
🧰 Additional context used📓 Path-based instructions (1)Flag potential security vulnerabilities Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries Flag unused or redundant code Flag outdated or incorrect comments/docstrings Ensure new code handles err...⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthrough
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The CA and certificate detail API contracts are preserved; no actionable merge risk is evident. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title accurately describes the PKI API query-count regression and its fix, but the prefix is invalid. The requirements allow one prefix such as [fix] or [chores], while the title uses the combined prefix [chores:fix].
Comment |
|
Proposed change log entry: |
Checklist
Description of Changes
Two tests are currently failin in CI with an increased query count:
test_ca_put_api— 8 instead of 6test_cert_put_api— 11 instead of 10See: https://github.com/openwisp/openwisp-controller/actions/runs/37550871419/job/112565767578.
Reason
We have set the common_name field in the
CaandCertmodels with a unique constraint on("common_name", "organization"), so the uniqueness is already checked by the model and enforced by the database.With the latest DRF 3.18 update in openwisp-utils,
common_nameis picked up by the serializer'sUniqueTogetherValidatorfor("common_name", "organization"), so it checks this uniqueness again. Every PUT now re-checks it at the serializer level on top of the model's ownUniqueConstraintcheck, which costs 2 extra queries per request:organizationSELECTto probe for an existing(common_name, organization)rowIntroduced in DRF 3.18.2.
Solution
Declare
common_nameexplicitly as a read-only field inCaDetailSerializerandCertDetailSerializer. It is not editable aftercreation anyway, and the value is already validated by the model, so the
duplicate check and its 2 queries are avoided.
Alternatively, if we would rather not add a read-only field, we can simply
bump the expected query counts in the two tests (6 → 8 and 10 → 11) and
accept the 2 extra queries per request.
Let me know if the second one is better; I have gone ahead with the first.
How to verify
Make sure the virtualenv has DRF = 3.18.3 then remove the two
common_namelines fromopenwisp_controller/pki/api/serializers.pyand run testsBoth tests fail with the counts above; with the fix in place, the suite passes.