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.

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

Fix logentry.channel json marshaling/unmarshaling #264

Merged
merged 2 commits into from
Feb 24, 2021

Conversation

StupidScience
Copy link
Contributor

Hello.

I've faced an issue when unmarshalling and marshalling back of LogEntry lead to results that differ from initial.

First of all just because of absent json tags, so it lead to "Type" instead of "type", so the next unmarshaling was broken.
Also "fixing" tags leads to nested object and

{
    ...
    "channel": {
	    "type": "web_trigger",
	    "summary": "My new incident",
	    "details_omitted": false
    }
}

transforms to

{
    ...
    "channel": {
	    "type": "web_trigger",
            "raw": {
	        "summary": "My new incident",
	        "details_omitted": false
            }
    }
}

So this PR will fix this, however it require Channel type changes.

Looking forward for your feedback.

@theckman
Copy link
Collaborator

theckman commented Feb 20, 2021

So this change is technically breaking API compatibility, and under normal circumstances would be a candidate for a v2.0.0 release. That said, digging further I'm not confident that the old format was actually functional based on the API documentation. As such, I'm not confident we should hold this to the same compatibility guarantee under the v1.0.0 release.

I'm going to sync up with @stmcallister to propose a v1.5.0 release that includes this change, and other "breaking" changes that fix an implementation that has never worked reliably.

@StupidScience
Copy link
Contributor Author

@theckman may be better instead of breaking API would be to provide custom MarshalJSON() function? It won't break things but should resolve issue as well. WDYT?

@theckman
Copy link
Collaborator

theckman commented Feb 21, 2021

Yeah, that may be more ideal. Unmarshal into a map, extract the type field out, and then set the rest of the map on the Raw field.

@theckman theckman modified the milestones: v1.5.0, v1.4.0 Feb 22, 2021
Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have made that comment (^) while requesting changes.

Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting two final adjustments, one of them is my fault. Afterwards I think this will be 👍

log_entry.go Outdated
@@ -115,8 +115,27 @@ func (c *Channel) UnmarshalJSON(b []byte) error {
ct, ok := raw["type"]
if ok {
c.Type = ct.(string)
delete(raw, "type")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm going to be a real pain in your rear here... For some reason it didn't dawn on me that removing this would also be a breaking change, and also make it not the raw object anymore. Would you have the spare moment to squash out this line? If not I can fix it after merging this PR.

Thank you so much for this contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem at all. I'll send update shortly

log_entry.go Outdated

b, err := json.Marshal(raw)
if err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect json.Marshal() effectively does the same thing. How would you feel about simplifying this error handling block all down to return json.Marshal(raw)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I'll fix it

@theckman theckman merged commit 5bedfd1 into PagerDuty:master Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants