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

omitempty should be set for ContentTypes and Extensions #282

Merged
merged 1 commit into from
Jul 6, 2021
Merged

omitempty should be set for ContentTypes and Extensions #282

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions fastly/gzip.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ type CreateGzipInput struct {
ServiceVersion int

Name string `form:"name,omitempty"`
ContentTypes string `form:"content_types"`
Extensions string `form:"extensions"`
ContentTypes string `form:"content_types,omitempty"`
Extensions string `form:"extensions,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there ever a scenario where we want to accept empty strings? If so, then we'd want the type to be a pointer reference so we can distinguish between a field not set vs a field that was explicitly set to an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, thanks for asking that. Let me test a bit more and will follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Integralist Confirmed that we don't need to accept empty strings for CreateGzipInput struct. If content_types and extensions are not specified in a POST request, the default values are assigned automatically by the API backend.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies in the slow reply.

The scenario I was curious about was more about does it make sense for content_types and extensions to be set to an empty value. For example, if I use the gzip CREATE api endpoint directly (e.g. not using go-fastly) and I pass:

extensions=html+css+js
content_types=

Would the API still create a default for content_types or would it attempt to content_types to an empty value? And would that be something we expect users to do?

In the above example I'm trying to say I only care about gzipping files with the extensions 'html', 'css' and 'js' and I don't really care about gzipping responses that have a certain content_type.

But I guess it doesn't really matter if the API has provided its own defaults for content_types because if all I care about is what's in extensions then that should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Integralist I believe I sent a link in DM that refers to our internal PR.

Would the API still create a default for content_types or would it attempt to content_types to an empty value?

I mentioned this in that PR, but we also had a bug in our API backend that we fail to set our default value if an empty value is sent for those parameters. This is going to be fixed very soon once that PR is deployed.

That said, now this public PR doesn't really matter since we also work around that in our API backend as well.
But, in that sense, since sending an empty value would produce the exact same result as not sending it, there's now zero reason users want to send an empty value and hence omitempty is an appropriate option here.

CacheCondition string `form:"cache_condition,omitempty"`
}

Expand Down