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

resources: define string values and keys as urlencoded #505

Closed
wants to merge 1 commit into from
Closed

resources: define string values and keys as urlencoded #505

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
3 changes: 3 additions & 0 deletions specification/sdk-resource.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ Required parameters:
- a collection of name/value attributes, where name is a string and value can be one
of: string, int64, double, bool.

The key and value, if value is a string, are trimmed, url encoded strings and
Copy link
Member

Choose a reason for hiding this comment

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

This seems like an implementation detail / something the exporter should do if required. If I have an exporter that supports arbitrary strings as keys and values then I don't want to urlencode -- I might even have to urldecode in the exporter.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is also important for when reading in from the environment, or other sources.

If the value is simply defined as urlencoded then it makes dealing with these simpler and defined all at once.

Copy link
Member

@Oberon00 Oberon00 Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Well, not really IMHO. If we just define that values are URL-encoded then I will still need to urldecode the environment variable to avoid double-encoding when I pass the parsed values to the resource constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is defined as url-encoded then you know not to do an encoding so there is no need to decode.

Copy link
Member

Choose a reason for hiding this comment

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

So who is responsible for doing the encoding?

Copy link
Member Author

@tsloughter tsloughter Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so just adding a MUST be url encoded strings?

--

Edited to fix SHOULD to MUST

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @Oberon00 , this seems misplaced. In most cases the attributes will be transmitted either via proto or via JSON, neither of which require URL encoding, which only increases the size. The situation with reading ENV vars is an example of another externalized representation, which also happens to have other rules (like it's a comma-separated list). So URL-encoding may be applied to the definition of that externalized representation, not to the API or how the values are represented internally.

Copy link
Member

Choose a reason for hiding this comment

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

"SHOULD be" is still bad. If that is true, then nothing changes, i.e. that statement has no effect. The question the spec should answer is who (implementation? caller?) should do what (encode? decode?) and when (at export? when creating resources? at the backend?).

Copy link
Member Author

Choose a reason for hiding this comment

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

oops. I meant MUST

Copy link
Member

Choose a reason for hiding this comment

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

My reply is about the "be" though, should vs must is not my issue.

case sensitive.

### Merge

The interface MUST provide a way for a primary resource to merge with a
Expand Down