Issue 113490043: Added support for URI Templates expansion (Closed)
|
Created:
11 years, 4 months ago by tmatsuo(Google) Modified:
11 years, 3 months ago CC:
gmlewis1, bradfitz, golang-codereviews Visibility:
Public. |
DescriptionAdded support for URI Templates expansion.
This is a fix for:
https://code.google.com/p/google-api-go-client/issues/detail?id=65
Patch Set 1 #
Total comments: 10
Patch Set 2 : Bundled uritemplates, and added fallback to the original Expand #Patch Set 3 : Removed stale comment and code #
Total comments: 10
Patch Set 4 : Moved uritemplates to internal directory and some refactorings #
Total comments: 3
Patch Set 5 : Add a link for RFC6570 #Patch Set 6 : Comment about double slashes #
Total comments: 2
Patch Set 7 : A correction in regexp #
Total comments: 2
Patch Set 8 : Removed an unnecessary cast #Patch Set 9 : Re-added mistakingly removed files #
Total comments: 6
Patch Set 10 : Removed the fallback method and unnecessary regexps #
Total comments: 6
Patch Set 11 : Removed unnecessary bits from the code #
MessagesTotal messages: 21
In addition to the comments below, can you please add unit tests for the modified functionality? https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.go File google-api-go-generator/gen.go (right): https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:42: useURITemplates = flag.Bool("useuritemplates", false, "Use 3rd party URI Templates library") I think this flag would be easier to read with underscores, like "user_uri_templates". But do we want to be calling the generator with different flags on different APIs? https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:416: pn("\turitemplates \"github.com/jtacoma/uritemplates\"") No need to rename the import here. Just the latter part is fine. https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:1243: pn(`template, _ := uritemplates.Parse(req.URL.Path)`) If parsing can fail, can we fall back on the googleapi.Expand version? https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:1245: pn(`expanded, _ := template.Expand(map[string]interface{}{`) Also, if template.Expand can fail, can we also fall back on the googleapi.Expand version? https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:1254: for _, arg := range argsForLocation { Since this loop is the same for both parts of the 'if', it would make it easier to share this part of the code and then gracefully fall back to the googleapi.Expand if there were any problems using the uritemplates package.
Sign in to reply to this message.
I don't think this warrants a new dependency. Especially not as a flag, as there is only one real user of the generator code: us, who run it to make the auto-generated code. I'd rather vendor it, if the license is compatible, or reimplement it, since it's all pretty trivial and well-specified.
Sign in to reply to this message.
PTAL Agreed with the comment from Brad about dependency, so I incorporated the snapshot of the uritemplates in the "googleapi/uritemplates" directory. Sorry if I misunderstand you Brad. I also added some unit tests for ensuring compatibility. However, I'm still worrying my code can break existing libraries with the next auto generation. Let me know if you have a good idea for how to ensure we won't break anything. Thanks, https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.go File google-api-go-generator/gen.go (right): https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:42: useURITemplates = flag.Bool("useuritemplates", false, "Use 3rd party URI Templates library") On 2014/07/24 02:55:29, gmlewis1 wrote: > I think this flag would be easier to read with underscores, like > "user_uri_templates". > But do we want to be calling the generator with different flags on different > APIs? Agreed for naming, but I removed the flag anyways. I originally introduced this flag because * I don't want to break anything * Using this flag introduces a dependency for 3rd party library However, I ended up incorporating uritemplates and adding unit tests for the new function. https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:416: pn("\turitemplates \"github.com/jtacoma/uritemplates\"") On 2014/07/24 02:55:29, gmlewis1 wrote: > No need to rename the import here. Just the latter part is fine. Removed this bit. https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:1243: pn(`template, _ := uritemplates.Parse(req.URL.Path)`) On 2014/07/24 02:55:29, gmlewis1 wrote: > If parsing can fail, can we fall back on the googleapi.Expand version? Done with the new ExpandWithURITemplates function. https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:1245: pn(`expanded, _ := template.Expand(map[string]interface{}{`) On 2014/07/24 02:55:29, gmlewis1 wrote: > Also, if template.Expand can fail, can we also fall back on the googleapi.Expand > version? Ditto https://codereview.appspot.com/113490043/diff/1/google-api-go-generator/gen.g... google-api-go-generator/gen.go:1254: for _, arg := range argsForLocation { On 2014/07/24 02:55:29, gmlewis1 wrote: > Since this loop is the same for both parts of the 'if', it would make it easier > to share this part of the code and then gracefully fall back to the > googleapi.Expand if there were any problems using the uritemplates package. Please see the new ExpandWithURITemplates function.
Sign in to reply to this message.
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi.go#ne... googleapi/googleapi.go:22: "strings" standard library first. blank line after this, and then the "code.google.com/p/google-api-go-client/googleapi/internal/uritemplates" one https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi.go#ne... googleapi/googleapi.go:338: func ExpandWithURITemplates(u *url.URL, expansions map[string]string) { why do we need both this and Expand? Just name this one Expand. The existing Expand tests should still pass I hope. https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi_test.... googleapi/googleapi_test.go:179: var expandTests = append(expandCommonTests, ExpandTest{ why append? https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi_test.... googleapi/googleapi_test.go:188: var expandWithURITemplatesTests = append(expandCommonTests, ExpandTest{ this isn't safe. you're potentially overwriting memory from the previous append. Go read about how append actually works. https://codereview.appspot.com/113490043/diff/40001/googleapi/uritemplates/ur... File googleapi/uritemplates/uritemplates.go (right): https://codereview.appspot.com/113490043/diff/40001/googleapi/uritemplates/ur... googleapi/uritemplates/uritemplates.go:1: // Copyright 2013 Joshua Tacoma. All rights reserved. Please vendor this under a directory named "internal" : googleapi/internal/uritemplates/uritemplates.go Go 1.4 will support internal packages for the standard library and we suspect Go 1.5 will support them for external packages like this, so we're already ready if we do this now. It also signals to other people to not use this vendored copied directly, and signals to tools like godoc.org to not render these in their docs/search.
Sign in to reply to this message.
Thanks for the review Brad!
PTAL
This time, I'm pretty sure we won't break any existing libraries because we use
uritemplates lib only when there are some special operators like "{+propname}".
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi.go
File googleapi/googleapi.go (right):
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:22: "strings"
On 2014/07/24 16:10:09, bradfitz wrote:
> standard library first.
>
> blank line after this, and then the
> "code.google.com/p/google-api-go-client/googleapi/internal/uritemplates" one
Done.
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:338: func ExpandWithURITemplates(u *url.URL, expansions
map[string]string) {
On 2014/07/24 16:10:09, bradfitz wrote:
> why do we need both this and Expand? Just name this one Expand.
>
> The existing Expand tests should still pass I hope.
Thanks! Agreed and done.
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi_test.go
File googleapi/googleapi_test.go (right):
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi_test....
googleapi/googleapi_test.go:179: var expandTests = append(expandCommonTests,
ExpandTest{
On 2014/07/24 16:10:09, bradfitz wrote:
> why append?
Because I'm super novice
https://codereview.appspot.com/113490043/diff/40001/googleapi/googleapi_test....
googleapi/googleapi_test.go:188: var expandWithURITemplatesTests =
append(expandCommonTests, ExpandTest{
On 2014/07/24 16:10:09, bradfitz wrote:
> this isn't safe. you're potentially overwriting memory from the previous
append.
> Go read about how append actually works.
Thanks for the info!
I'll read the article about append later, while I'm removing these append calls.
https://codereview.appspot.com/113490043/diff/40001/googleapi/uritemplates/ur...
File googleapi/uritemplates/uritemplates.go (right):
https://codereview.appspot.com/113490043/diff/40001/googleapi/uritemplates/ur...
googleapi/uritemplates/uritemplates.go:1: // Copyright 2013 Joshua Tacoma. All
rights reserved.
Thanks for the info, and done.
On 2014/07/24 16:10:10, bradfitz wrote:
> Please vendor this under a directory named "internal" :
>
> googleapi/internal/uritemplates/uritemplates.go
>
> Go 1.4 will support internal packages for the standard library and we suspect
Go
> 1.5 will support them for external packages like this, so we're already ready
if
> we do this now. It also signals to other people to not use this vendored
copied
> directly, and signals to tools like http://godoc.org to not render these in
their
> docs/search.
Sign in to reply to this message.
https://codereview.appspot.com/113490043/diff/60001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:333: values := make(map[string]interface{}) Brad, would this be a place where var values map[string]interface{} would be preferred to indicate that it is for internal use only? https://codereview.appspot.com/113490043/diff/60001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/113490043/diff/60001/googleapi/googleapi_test.... googleapi/googleapi_test.go:191: "http://www.golang.org//topics/myproject/mytopic", Do we really want a double-slash here at "...//topics/..."?
Sign in to reply to this message.
https://codereview.appspot.com/113490043/diff/60001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/113490043/diff/60001/googleapi/googleapi_test.... googleapi/googleapi_test.go:191: "http://www.golang.org//topics/myproject/mytopic", On 2014/07/25 01:30:47, gmlewis1 wrote: > Do we really want a double-slash here at "...//topics/..."? Yes, I think it's weird too, but yes :) (Otherwise the API call will fail)
Sign in to reply to this message.
On 2014/07/25 01:43:34, tmatsuo(Google) wrote: > Yes, I think it's weird too, but yes :) > (Otherwise the API call will fail) In that case, can you document that somewhere? I don't remember seeing that explained... but maybe I missed it?
Sign in to reply to this message.
On 2014/07/25 01:52:00, gmlewis1 wrote: > On 2014/07/25 01:43:34, tmatsuo(Google) wrote: > > Yes, I think it's weird too, but yes :) > > (Otherwise the API call will fail) > > In that case, can you document that somewhere? I don't remember seeing that > explained... but maybe I missed it? I added a link for RFC6570. Hope it works for you, and let me know otherwise :)
Sign in to reply to this message.
LGTM, but please wait for final review from Brad. Thanks, Takashi!
Sign in to reply to this message.
https://codereview.appspot.com/113490043/diff/100001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/100001/googleapi/googleapi.go#n... googleapi/googleapi.go:319: var findURITemplatesOperator = regexp.MustCompile(`(\{[\+\./;\?\&\#]+[A-Za-z_]+\})`) why is this [\+\./;\?\&\#]+ instead of [\+\./;\?\&\#] ? In http://tools.ietf.org/html/rfc6570 I don't see any type of {...} that has two leading symbols. Also, why isn't it [\+\./;\?\&\#]? meaning 0 or 1, so it also handles http://tools.ietf.org/html/rfc6570#section-3.2.2? Then we don't need the "Fallback to the original method." section below. Doesn't the uritemplates package we're vendoring here do the simple case?
Sign in to reply to this message.
Sorry for my slow response. I got a bereavement. PTAL https://codereview.appspot.com/113490043/diff/100001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/100001/googleapi/googleapi.go#n... googleapi/googleapi.go:319: var findURITemplatesOperator = regexp.MustCompile(`(\{[\+\./;\?\&\#]+[A-Za-z_]+\})`) On 2014/07/25 18:00:41, bradfitz wrote: > why is this [\+\./;\?\&\#]+ instead of [\+\./;\?\&\#] ? In > http://tools.ietf.org/html/rfc6570 I don't see any type of {...} that has two > leading symbols. Good catch. Done. > > Also, why isn't it [\+\./;\?\&\#]? meaning 0 or 1, so it also handles > http://tools.ietf.org/html/rfc6570#section-3.2.2? Then we don't need the > "Fallback to the original method." section below. Doesn't the uritemplates > package we're vendoring here do the simple case? I'm pretty sure that the uritemplates package does the simple case, but I don't want to break any existing libraries with this change. That said, I agree that having this fallback code snippet is kind of a technical debt. I'll first try to work with the existing contributors (especially people who sent us patches in the past) to make sure the completeness of the current test cases, and then eliminate this fallback method. I'll do it in another CL though. SG? Also, Glenn wanted to have this fallback upon failure cases.
Sign in to reply to this message.
On 2014/08/02 21:38:16, tmatsuo(Google) wrote: > Sorry for my slow response. I got a bereavement. > > PTAL > > https://codereview.appspot.com/113490043/diff/100001/googleapi/googleapi.go > File googleapi/googleapi.go (right): > > https://codereview.appspot.com/113490043/diff/100001/googleapi/googleapi.go#n... > googleapi/googleapi.go:319: var findURITemplatesOperator = > regexp.MustCompile(`(\{[\+\./;\?\&\#]+[A-Za-z_]+\})`) > On 2014/07/25 18:00:41, bradfitz wrote: > > why is this [\+\./;\?\&\#]+ instead of [\+\./;\?\&\#] ? In > > http://tools.ietf.org/html/rfc6570 I don't see any type of {...} that has two > > leading symbols. > > Good catch. Done. > > > > > Also, why isn't it [\+\./;\?\&\#]? meaning 0 or 1, so it also handles > > http://tools.ietf.org/html/rfc6570#section-3.2.2? Then we don't need the > > "Fallback to the original method." section below. Doesn't the uritemplates > > package we're vendoring here do the simple case? > > I'm pretty sure that the uritemplates package does the simple case, but I don't > want to break any existing libraries with this change. That said, I agree that > having this fallback code snippet is kind of a technical debt. > > I'll first try to work with the existing contributors (especially people who > sent us patches in the past) to make sure the completeness of the current test > cases, and then eliminate this fallback method. I'll do it in another CL though. > > SG? > > Also, Glenn wanted to have this fallback upon failure cases. Friendly ping
Sign in to reply to this message.
LGTM, but please let Brad perform the final review. https://codereview.appspot.com/113490043/diff/120001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:335: values[k] = interface{}(v) Is it possible to just say `values[k] = v` here?
Sign in to reply to this message.
Brad, can you take another look? https://codereview.appspot.com/113490043/diff/120001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:335: values[k] = interface{}(v) On 2014/08/09 04:43:10, gmlewis1 wrote: > Is it possible to just say `values[k] = v` here? Yes, and done.
Sign in to reply to this message.
https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go#n... googleapi/googleapi.go:316: var findEncodedStrings = regexp.MustCompile(`(\{[A-Za-z_]+\})`) seems like these two regexps should be in the uritemplates package https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go#n... googleapi/googleapi.go:328: match := findURITemplatesOperator.MatchString(u.Path) and this code should be in the uritemplates package this function could be like one line or so, just calling into the uritemplates package. https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go#n... googleapi/googleapi.go:346: // Fallback to the original method. I still don't like this. If we're importing a URI Templates package, it should just work or we should make it work. We don't need two copies of it, both incomplete. Please add enough tests to give you confidence that the new version sufficiently replaces the old version.
Sign in to reply to this message.
PTAL I made a small change on one of the tests for a case when the expansion is not found on the map. In the old code, the original URI is unchanged, but in the new code, the uritemplates variable will be replaced with an empty string. I don't think any APIs expect the old behavior, but let me know you think otherwise. https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go#n... googleapi/googleapi.go:316: var findEncodedStrings = regexp.MustCompile(`(\{[A-Za-z_]+\})`) On 2014/08/13 21:15:24, bradfitz wrote: > seems like these two regexps should be in the uritemplates package Agreed, and actually we don't need this if we just use uritemplates package. https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go#n... googleapi/googleapi.go:328: match := findURITemplatesOperator.MatchString(u.Path) On 2014/08/13 21:15:24, bradfitz wrote: > and this code should be in the uritemplates package > > this function could be like one line or so, just calling into the uritemplates > package. Got it and done. https://codereview.appspot.com/113490043/diff/160001/googleapi/googleapi.go#n... googleapi/googleapi.go:346: // Fallback to the original method. On 2014/08/13 21:15:24, bradfitz wrote: > I still don't like this. > > If we're importing a URI Templates package, it should just work or we should > make it work. We don't need two copies of it, both incomplete. > > Please add enough tests to give you confidence that the new version sufficiently > replaces the old version. Alright, done. I took the second look on the googleapi_test.go and it looks fine to me.
Sign in to reply to this message.
https://codereview.appspot.com/113490043/diff/180001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:315: // the map supplied, using uritemplates package. remove this comment change. This package isn't even accessible to end users so it's just an internal implementation detail. https://codereview.appspot.com/113490043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:323: return this line is unnecessary https://codereview.appspot.com/113490043/diff/180001/googleapi/internal/urite... File googleapi/internal/uritemplates/utils.go (right): https://codereview.appspot.com/113490043/diff/180001/googleapi/internal/urite... googleapi/internal/uritemplates/utils.go:12: expanded, err := template.Expand(values) just: return template.Expand(values) ?
Sign in to reply to this message.
Thanks Brad, PTAL https://codereview.appspot.com/113490043/diff/180001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/113490043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:315: // the map supplied, using uritemplates package. On 2014/08/18 18:02:52, bradfitz wrote: > remove this comment change. This package isn't even accessible to end users so > it's just an internal implementation detail. Done. https://codereview.appspot.com/113490043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:323: return On 2014/08/18 18:02:52, bradfitz wrote: > this line is unnecessary Done. https://codereview.appspot.com/113490043/diff/180001/googleapi/internal/urite... File googleapi/internal/uritemplates/utils.go (right): https://codereview.appspot.com/113490043/diff/180001/googleapi/internal/urite... googleapi/internal/uritemplates/utils.go:12: expanded, err := template.Expand(values) On 2014/08/18 18:02:52, bradfitz wrote: > just: > > return template.Expand(values) > > ? Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/google-api-go-client/source/detail?r=11e893087017 *** Added support for URI Templates expansion. Fixes Issue 65 LGTM=gmlewis, bradfitz R=gmlewis, bradfitz CC=golang-codereviews https://codereview.appspot.com/113490043 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
