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

Validate application name #1019

Merged
merged 2 commits into from
May 30, 2017

Conversation

chrisdunelm
Copy link
Contributor

Validated on setting in Initializer. Fixes #729

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 26, 2017
Validated on setting in Initializer. Fixes googleapis#729.
get => _applicationName;
set
{
if (value != null && !ProductInfoHeaderValue.TryParse(value, out var _))

This comment was marked as spam.

This comment was marked as spam.

@chrisdunelm chrisdunelm changed the base branch from master to v127wip May 28, 2017 08:00
Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Gosh - I'd never seen that class before. I assume you've checked that it represents the same set of valid values as the servers check?

@chrisdunelm
Copy link
Contributor Author

This validates that the value is possible/legal to send in the UserAgent header. It's possible the server is more restrictive than this, but this at least ensures the value will get to the server.

@chrisdunelm chrisdunelm merged commit a9dbf3d into googleapis:v127wip May 30, 2017
@chrisdunelm chrisdunelm deleted the appnamevalidation branch May 30, 2017 11:57
chrisdunelm added a commit that referenced this pull request Jun 1, 2017
* Provide synchronous access to default credentials (#1018)

Fixes #652

* Validate application name (#1019)

Fixes #729.

* Add download method that returns final download status (#1020)

Fixes #982.

* Support range downloads in generated code (#1021)

Fixes #981

* Unregister CancellationToken, and fix exception handling on cancellation (#1023)

Fixes #1022

* Clarify NET Core support (#1027)

Fixes #1025

* Update version v1.26.2 -> v1.27.0 (#1028)

* Support JWT validation (#1026)

Fixes #920

* Real JWT validation integration test (#1032)
@LindaLawton
Copy link
Collaborator

LindaLawton commented Jun 7, 2017

Me being curious again:

How do you decide what you want to validate and what you want to just return an error to the user? Isn't it over kill to be checking that its valid every time? Why not just send it and if once out of a thousand it doesn't work then send a nice message to the user that it wont work probably because it cant be sent as a user agent? and tell them it needs to be rfc2616

I realize that checking it probably doesn't take much power but if you check something 1000 times to prevent one fail. Is checking a waste of time?

@jskeet
Copy link
Collaborator

jskeet commented Jun 7, 2017

@LindaLawton: It's only validated on client initialization, not on every request.

But we've seen it be a tricky issue to diagnose before now, and upfront validation allows us to give a much more specific error.

@LindaLawton
Copy link
Collaborator

I have only heard of this issue popping up once or twice in the last five years, so was unaware that it was a bigger issue.

Once again thanks for responding to my curiosity its interesting to hear how other developers approach issues, and how you decide what to address and how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants