API: prevent duplicate records in single RRset #4195
Conversation
zeha
commented
Jul 15, 2016
•
zeha
commented
- read the CONTRIBUTING.md document
- compiled and tested this code
- included documentation (including possible behaviour changes)
- documented the code
- added regression tests
- added unit tests
|
Somebody please review this. |
|
looks good to me, please rebase 👍 |
|
May I please object? Although this might be technically correct, it is not very userfriendly. When importing a zone from another setup, I would just like to AXFR the zone, copy paste it into nsedit and be done with it. If I choose not to send an array with nameservers, just use the nameservers that are already in the zone, or b0rk. If I however do send an array with nameservers, override the nameservers in the zone. If you b0rk on nameservers in the array and in the zone, you are forcing users to first review the whole zone before they are able to import. So please reconsider this. |
|
@tuxis-ie This would imply the outcome of sending conflicting data is a silent loss of either data. If you want a fallback, client code can handle this. (Either by looking at the data before sending, or by sending 'replace NS' afterwards.) |
zeha
force-pushed
the
api-nameservers-or-ns-rrset
branch
from
06f76c5
to
8b3d29a
Compare
|
@pieterlexis rebased. |
zeha
force-pushed
the
api-nameservers-or-ns-rrset
branch
from
8b3d29a
to
3443eaf
Compare
|
Rebased once more. |
pdns/ws-auth.cc
Outdated
| static void checkDuplicateRecords(vector<DNSResourceRecord>& records) { | ||
| sort(records.begin(), records.end(), | ||
| [](const DNSResourceRecord& rec_a, const DNSResourceRecord& rec_b) -> bool { | ||
| return rec_a.qname.toString() > rec_b.qname.toString() || rec_a.qtype.getCode() > rec_b.qtype.getCode(); |
There was a problem hiding this comment.
Content must be part of the sorting, without it there is no guarantee duplicates are sequential if there are 3 or more records with the same "name type" Could use a test.
Is qname always lower case here?
zeha
force-pushed
the
api-nameservers-or-ns-rrset
branch
from
3443eaf
to
b391583
Compare
Avoids firewall warning on OS X, and is generally good.
zeha
force-pushed
the
api-nameservers-or-ns-rrset
branch
from
b391583
to
85aba43
Compare
If a zone already had duplicates, we do nothing to them (for now).
zeha
force-pushed
the
api-nameservers-or-ns-rrset
branch
from
85aba43
to
e3675a8
Compare
|
Worked in review feedback + rebased. |
| [](const DNSResourceRecord& rec_a, const DNSResourceRecord& rec_b) -> bool { | ||
| return rec_a.qname.toString() > rec_b.qname.toString() || \ | ||
| rec_a.qtype.getCode() > rec_b.qtype.getCode() || \ | ||
| rec_a.content < rec_b.content; |
There was a problem hiding this comment.
Has content been canonicalized at this point?
