DnsProvider.create: fix Domain key mismatch and roll back on failure (#129)
Two compounding bugs, both hit while adding a DuckDNS provider: 1. Domain.get() normalized every lookup via tldExtract before checking Redis. model-redis' Table.create() stores the new record under the literal key it's given, then calls this.get() internally to return the created instance — so for any domain tldExtract doesn't recognize as a shared/public suffix (e.g. a DuckDNS name like "myhost.duckdns.org", which tldExtract naively normalizes to "duckdns.org"), that final read-back always missed and create() threw EntryNotFound, despite the record having just been written successfully. In other words: no *.duckdns.org domain could ever be created. Fixed by trying the exact string first and only falling back to the tldExtract-normalized parent if no exact record exists — preserving the original "look up an arbitrary hostname, find the Domain that governs it" behavior for real lookups, while fixing create()'s own read-back of what it just wrote. 2. create() saves the DnsProvider row first, then calls updateDomains() as a separate step. If updateDomains() throws — e.g. a Domain collides with a stale/orphaned record left over from an earlier failed attempt (exactly what bug 1 was silently producing) — the already-saved provider row was never cleaned up, leaving a broken, domain-less provider behind despite the API returning an error. Fixed by wrapping updateDomains() in its own try/catch and removing the provider on failure. That fix has its own subtlety: `instance` (from super.create()) has its `domains` relation resolved by super.create()'s own internal get() call, which runs BEFORE updateDomains() creates any Domain rows — so instance.domains is permanently stale (always empty), on both the success and failure paths. Removing `instance` directly would delete the provider but silently leave behind whatever domains updateDomains() did manage to create. Fixed by re-fetching (this.get(instance.id)) before both the success return and the failure-path remove(), so relations are current in both cases — the returned/API-response instance and the rollback's cascade-delete. Manually verified against a live Redis (this project's test philosophy explicitly excludes Redis-ORM-dependent tests from the automated suite — see test/README.md "Philosophy"): - A pre-existing orphaned Domain (simulating bug 1's fallout) now produces an accurate "already exists" error instead of a confusing EntryNotFound for the wrong (normalized) domain name, and the failed create() leaves zero orphaned providers behind. - A genuinely new *.duckdns.org domain now creates successfully, correctly links to its provider, and is fully cascade-deleted when the provider is removed. - npm test: 192/192 pass.
This commit is contained in:
@@ -29,12 +29,35 @@ class Domain extends Table{
|
|||||||
'zoneId': {isRequired: false, type: 'string'},
|
'zoneId': {isRequired: false, type: 'string'},
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Try the exact string first — Domain.create() stores records under the
|
||||||
|
// literal domain it's given (see model-redis' Table.create, which HSETs
|
||||||
|
// under data[this._key] verbatim), and its own final `this.get(...)` to
|
||||||
|
// return the created instance must find that same literal key. Falling
|
||||||
|
// straight to tldExtract normalization here breaks that for any domain
|
||||||
|
// tldExtract doesn't recognize as a shared/public suffix — e.g. a DuckDNS
|
||||||
|
// name like "myhost.duckdns.org" normalizes to "duckdns.org" (tldExtract
|
||||||
|
// has no idea duckdns.org is shared across many independent registrants),
|
||||||
|
// so create() would always throw EntryNotFound on its own read-back
|
||||||
|
// despite the record having just been written successfully.
|
||||||
|
// Only fall back to the normalized parent domain if no exact record
|
||||||
|
// exists — that's what lets callers look up an arbitrary hostname (e.g.
|
||||||
|
// "www.example.com") and find the Domain that governs it (example.com).
|
||||||
static async get(domain, ...args){
|
static async get(domain, ...args){
|
||||||
try{
|
try{
|
||||||
domain = tldExtract(domain).domain;
|
return await super.get(domain, ...args);
|
||||||
}catch{}
|
}catch(error){
|
||||||
|
if(error.name !== 'EntryNotFound') throw error;
|
||||||
|
|
||||||
return await super.get(domain, ...args);
|
let normalized;
|
||||||
|
try{
|
||||||
|
normalized = tldExtract(domain).domain;
|
||||||
|
}catch{
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
|
if(normalized === domain) throw error;
|
||||||
|
|
||||||
|
return await super.get(normalized, ...args);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
async getRecords(...args){
|
async getRecords(...args){
|
||||||
@@ -121,9 +144,32 @@ class DnsProvider extends Table{
|
|||||||
let domains = await provider.listDomains();
|
let domains = await provider.listDomains();
|
||||||
|
|
||||||
let instance = await super.create.call(__intraModel, data, ...args);
|
let instance = await super.create.call(__intraModel, data, ...args);
|
||||||
await instance.updateDomains(domains);
|
try{
|
||||||
|
await instance.updateDomains(domains);
|
||||||
|
}catch(updateError){
|
||||||
|
// Don't leave a half-created provider behind (e.g. a Domain that
|
||||||
|
// collided with a stale/orphaned record from a previous failed
|
||||||
|
// attempt — see updateDomains' own comment on zoneId omission for
|
||||||
|
// another case this can throw). Re-fetch before removing: `instance`
|
||||||
|
// had its `domains` relation resolved by super.create()'s own
|
||||||
|
// internal get() BEFORE updateDomains() created any Domain rows, so
|
||||||
|
// it's permanently stale (always empty) — removing `instance`
|
||||||
|
// directly would delete the provider but silently leave behind
|
||||||
|
// whatever domains updateDomains() managed to create before failing,
|
||||||
|
// which is exactly the kind of orphan this is meant to prevent.
|
||||||
|
try{
|
||||||
|
await (await this.get(instance.id)).remove();
|
||||||
|
}catch(removeError){
|
||||||
|
console.error('DnsProvider create: failed to roll back', instance.id, 'after updateDomains error:', removeError.message);
|
||||||
|
}
|
||||||
|
throw updateError;
|
||||||
|
}
|
||||||
|
|
||||||
return instance;
|
// Same staleness issue as above: re-fetch so the returned instance
|
||||||
|
// (and the API response built from it) reflects the domains
|
||||||
|
// updateDomains() actually just created, not the empty snapshot from
|
||||||
|
// before it ran.
|
||||||
|
return await this.get(instance.id);
|
||||||
}catch(error){
|
}catch(error){
|
||||||
if(error.name === 'UnauthorizedDnsApi'){
|
if(error.name === 'UnauthorizedDnsApi'){
|
||||||
let keys = [];
|
let keys = [];
|
||||||
|
|||||||
Reference in New Issue
Block a user