Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UpdateRecord requires Name field #33

Closed
Ulexus opened this issue Jul 14, 2016 · 3 comments · Fixed by #92
Closed

UpdateRecord requires Name field #33

Ulexus opened this issue Jul 14, 2016 · 3 comments · Fixed by #92
Assignees
@weppos
Labels
bug Code defect or incorrect behavior

Comments

@Ulexus
Copy link

Ulexus commented Jul 14, 2016

The Name field of ZoneRecord does not have omitempty set on its tag, so it sends it even if it is empty, which causes the server to respond with 400 Validation failed.

The simple workaround is to always supply the Name field in the ZoneRecord, but this does not agree with the API documentation.

This should either be documented with UpdateRecord or have ZoneRecord updated to make the Name field tagged with omitempty.
@weppos
Copy link
Member

weppos commented Jul 14, 2016 •
edited
Loading

@Ulexus the omitempty was intentionally removed on #16 because we require to provide a blank "" string when you create a record for the apex domain.

I'm pretty sure it also worked for the updates at the time we tried, but indeed it seems to be a reasonable issue that I also pointed out at the time of #16.

We'll try to find a solution.

@Ulexus
Copy link
Author

Ulexus commented Jul 14, 2016 •
edited
Loading

I, too, think it worked originally. I had code out that suddenly stopped working at some point a few months ago, and I finally had time to track it down to this.

I don't think there is anything wrong with this compromise; I would just at least document it, since the requirement varies from the API spec. For my case, I was already calling GetRecord() first, anyway, so I just added the Name field to the update... I just needed to know that was required.

@weppos weppos mentioned this issue Oct 13, 2016
Merged
@weppos weppos added the bug Code defect or incorrect behavior label Aug 5, 2019
@weppos weppos mentioned this issue Apr 13, 2020
Closed
weppos added a commit that referenced this issue May 2, 2020
@weppos
Handle case where ZoneRecord name can be non present
b357415 
Closes GH-33
@weppos weppos mentioned this issue May 2, 2020
Merged
@weppos weppos self-assigned this May 2, 2020
@weppos
Copy link
Member

weppos commented May 2, 2020

Closed by #92

@weppos weppos closed this as completed in #92 May 2, 2020
weppos added a commit that referenced this issue May 2, 2020
@weppos
Handle case where ZoneRecord name can be not present (#92)
d209abf 
A ZoneRecord name can be an empty value `""`. This is because our API mimic the UI and it requires the consumer to specify the zone record name without the zone, in other words not the qualified name.

As a result, when the qualified name also matches the zone name (for apex), the value of name is blank:

```
zone: example
record: www.example.com
-> name: "www"

zone: example
record: example.com
-> name: ""
```

In Go, there is no way to distinguish a non-present string (null) vs a blank string. That's because [the zero value](https://tour.golang.org/basics/12) of a string type is an empty string.

I decided to not change the signature of ZoneRecord but instead supply a different type of record attributes. This is actually another quite common pattern for particularly complex requests, and we also use in the [Registrar](https://github.com/dnsimple/dnsimple-go/blob/master/dnsimple/registrar.go#L134) where we supply special input structs to handle the payload of the registrar calls. This time I went with `-Attributes` and not `-Input` as there is a direct relationship between this struct at the zone record attributes, as well the zone record struct.

Fixes #33
This was referenced May 4, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@weppos weppos

Labels
bug Code defect or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle case where ZoneRecord name can be not present dnsimple/dnsimple-go
2 participants
@weppos @Ulexus