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

Change the type of FileUpload's Links fld to FileLinkList #652

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Change the type of FileUpload's Links fld to FileLinkList #652

merged 1 commit into from
Aug 7, 2018

Conversation

spastorelli-stripe
Copy link
Contributor

@spastorelli-stripe spastorelli-stripe commented Aug 7, 2018 •
edited
Loading

The Links field of FileUpload struct is currently set to []*FileLink.
This can cause an unmarshalling error when calling for example the Create FileUpload method, as the FileUpload object return as response can include a list of file links:

{
  id: "file_xxxxxxxx",
  object: "file_upload",
  created: 1533640021,
  filename: "file.jpg",
  links: {
    object: "list",
    data: [ ... ],
    has_more: false,
    url: "/v1/file_links?file=file_xxxxxxxx"
  },
  ....
}

This PR attempts to fix this issue by changing the type of FileUpload's Links fld to FileLinkList.

r? @ob-stripe @brandur-stripe
cc @stripe/api-libraries

@spastorelli-stripe spastorelli-stripe changed the title [WIP] Change the type of FileUpload's Links fld to FileLinkList Change the type of FileUpload's Links fld to FileLinkList Aug 7, 2018
remi-stripe
remi-stripe reviewed Aug 7, 2018
View reviewed changes
Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

@spastorelli-stripe Thanks a lot for fixing. I left a minor comment, but once fixed you can re-assign to brandur and he'll take reviewing/releasing

filelink.go Outdated
@@ -53,7 +53,7 @@ func (c *FileLink) UnmarshalJSON(data []byte) error {
return nil
}

// FileLinkList is a list of file links as retrieved from a list endpoint.
// FileLinkList is a list object for file links.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep that comment since we tried to make it consistent across packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Reverted to the original comment.

fileupload.go
@@ -47,7 +47,7 @@ type FileUpload struct {
Created int64 `json:"created"`
ID string `json:"id"`
Filename string `json:"filename"`
Links []*FileLink `json:"links"`
Links *FileLinkList `json:"links"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, totally missed that one when I shipped the FileLink resource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't get the credit on this one as I didn't catch this. I only came across as it was reported in a ticket.

@brandur-stripe
Copy link
Contributor

Geeze, I should've caught that one on review too. Thanks @spastorelli-stripe!

Let me know if you've taken a look at Remi's comment, but will get this released ASAP after.

@spastorelli-stripe spastorelli-stripe force-pushed the spastorelli-fix-fileupload-linkslist-des branch from fe78297 to 9b107d7 Compare August 7, 2018 13:39
@spastorelli-stripe
Copy link
Contributor Author

Thanks @remi-stripe and @brandur-stripe for your reviews. I believe I have addressed Remi's comment. PTAL

@brandur-stripe
Copy link
Contributor

Thanks @spastorelli-stripe.

Even though this is an interface change, I'm going to release this as a patch because I don't think that the list of file links could ever have successfully deserialized into []*FileLink.

@brandur-stripe brandur-stripe merged commit 0a71e73 into master Aug 7, 2018
@brandur-stripe brandur-stripe deleted the spastorelli-fix-fileupload-linkslist-des branch August 7, 2018 13:47
@brandur-stripe
Copy link
Contributor

Released as 40.0.2.

nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
@mbianco-stripe
Move postmessage domain generation to functional style (#652)
45cdd62 
* Move postmessage domain generation to functional style

* Minor code cleanup

* Improvements to poll timestamp setup

* Fixing typing error

* Moving customer sanitization to sanitizer

* Drop dependabot to weekly to save CI time

* Shipping transformation fix

* Adding missing fields on customer test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remi-stripe remi-stripe remi-stripe left review comments

Assignees

@ob-stripe ob-stripe

@brandur-stripe brandur-stripe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spastorelli-stripe @brandur-stripe @remi-stripe @ob-stripe