code review 9958043: undo CL 9672043 / 38d1b743fd69 (issue 9958043)

[a...@golang.org]

Reviewers: campoy, bradfitz,

Message:
Hello cam...@google.com, brad...@golang.org (cc:
golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/google-api-go-client


Description:
undo CL 9672043 / 38d1b743fd69

I have had reports that this breaks user code.

««« original CL description
google-api-go-client: update generated code with new param encoding from
https://codereview.appspot.com/9671043/

R=adg, bradfitz
CC=golang-dev
https://codereview.appspot.com/9672043
»»»

Please review this at https://codereview.appspot.com/9958043/

Affected files:
M adexchangebuyer/v1.1/adexchangebuyer-api.json
M adexchangebuyer/v1.1/adexchangebuyer-gen.go
M adexchangebuyer/v1.2/adexchangebuyer-api.json
M adexchangebuyer/v1.2/adexchangebuyer-gen.go
M adexchangebuyer/v1/adexchangebuyer-api.json
M adexchangebuyer/v1/adexchangebuyer-gen.go
M adexchangeseller/v1/adexchangeseller-api.json
M adexchangeseller/v1/adexchangeseller-gen.go
R admin/directory_v1/admin-api.json
R admin/directory_v1/admin-gen.go
R admin/reports_v1/admin-api.json
R admin/reports_v1/admin-gen.go
M adsense/v1.1/adsense-api.json
M adsense/v1.1/adsense-gen.go
M adsense/v1.2/adsense-api.json
M adsense/v1.2/adsense-gen.go
M adsense/v1/adsense-api.json
M adsense/v1/adsense-gen.go
M adsensehost/v4.1/adsensehost-api.json
M adsensehost/v4.1/adsensehost-gen.go
M analytics/v2.4/analytics-api.json
M analytics/v2.4/analytics-gen.go
M analytics/v3/analytics-api.json
M analytics/v3/analytics-gen.go
M androidpublisher/v1/androidpublisher-api.json
M androidpublisher/v1/androidpublisher-gen.go
M audit/v1/audit-api.json
M audit/v1/audit-gen.go
M bigquery/v2/bigquery-api.json
M bigquery/v2/bigquery-gen.go
M blogger/v2/blogger-api.json
M blogger/v2/blogger-gen.go
M blogger/v3/blogger-api.json
M blogger/v3/blogger-gen.go
M books/v1/books-api.json
M books/v1/books-gen.go
M calendar/v3/calendar-api.json
M calendar/v3/calendar-gen.go
M civicinfo/us_v1/civicinfo-api.json
M civicinfo/us_v1/civicinfo-gen.go
M compute/v1beta13/compute-api.json
M compute/v1beta13/compute-gen.go
M compute/v1beta14/compute-api.json
M compute/v1beta14/compute-gen.go
R compute/v1beta15/compute-api.json
R compute/v1beta15/compute-gen.go
M coordinate/v1/coordinate-api.json
M coordinate/v1/coordinate-gen.go
M customsearch/v1/customsearch-api.json
M customsearch/v1/customsearch-gen.go
R datastore/v1beta1/datastore-api.json
R datastore/v1beta1/datastore-gen.go
M dfareporting/v1.1/dfareporting-api.json
M dfareporting/v1.1/dfareporting-gen.go
R dfareporting/v1.2/dfareporting-api.json
R dfareporting/v1.2/dfareporting-gen.go
M dfareporting/v1/dfareporting-api.json
M dfareporting/v1/dfareporting-gen.go
M discovery/v1/discovery-api.json
M discovery/v1/discovery-gen.go
M drive/v1/drive-api.json
M drive/v1/drive-gen.go
M drive/v2/drive-api.json
M drive/v2/drive-gen.go
M freebase/v1/freebase-api.json
M freebase/v1/freebase-gen.go
M freebase/v1sandbox/freebase-api.json
M freebase/v1sandbox/freebase-gen.go
M gan/v1beta1/gan-api.json
M gan/v1beta1/gan-gen.go
M groupsmigration/v1/groupsmigration-api.json
M groupsmigration/v1/groupsmigration-gen.go
M groupssettings/v1/groupssettings-api.json
M groupssettings/v1/groupssettings-gen.go
M latitude/v1/latitude-api.json
M latitude/v1/latitude-gen.go
M licensing/v1/licensing-api.json
M licensing/v1/licensing-gen.go
M mirror/v1/mirror-api.json
M mirror/v1/mirror-gen.go
M oauth2/v1/oauth2-api.json
M oauth2/v1/oauth2-gen.go
M oauth2/v2/oauth2-api.json
M oauth2/v2/oauth2-gen.go
M orkut/v2/orkut-api.json
M orkut/v2/orkut-gen.go
M pagespeedonline/v1/pagespeedonline-api.json
M pagespeedonline/v1/pagespeedonline-gen.go
M plus/v1/plus-api.json
M plus/v1/plus-gen.go
M prediction/v1.2/prediction-api.json
M prediction/v1.2/prediction-gen.go
M prediction/v1.3/prediction-api.json
M prediction/v1.3/prediction-gen.go
M prediction/v1.4/prediction-api.json
M prediction/v1.4/prediction-gen.go
M prediction/v1.5/prediction-api.json
M prediction/v1.5/prediction-gen.go
M reseller/v1/reseller-api.json
M reseller/v1/reseller-gen.go
M reseller/v1sandbox/reseller-api.json
M reseller/v1sandbox/reseller-gen.go
M shopping/v1/shopping-api.json
M shopping/v1/shopping-gen.go
M siteverification/v1/siteverification-api.json
M siteverification/v1/siteverification-gen.go
M storage/v1beta1/storage-api.json
M storage/v1beta1/storage-gen.go
M storage/v1beta2/storage-api.json
M storage/v1beta2/storage-gen.go
M taskqueue/v1beta1/taskqueue-api.json
M taskqueue/v1beta1/taskqueue-gen.go
M taskqueue/v1beta2/taskqueue-api.json
M taskqueue/v1beta2/taskqueue-gen.go
M tasks/v1/tasks-api.json
M tasks/v1/tasks-gen.go
M translate/v2/translate-api.json
M translate/v2/translate-gen.go
M urlshortener/v1/urlshortener-api.json
M urlshortener/v1/urlshortener-gen.go
M webfonts/v1/webfonts-api.json
M webfonts/v1/webfonts-gen.go
M youtube/v3/youtube-api.json
M youtube/v3/youtube-gen.go
M youtubeanalytics/v1/youtubeanalytics-api.json
M youtubeanalytics/v1/youtubeanalytics-gen.go
M youtubeanalytics/v1beta1/youtubeanalytics-api.json
M youtubeanalytics/v1beta1/youtubeanalytics-gen.go

[Brad Fitzpatrick]

LGTM but would prefer the commit reference a bug number with more details

[Andrew Gerrand]

added "Update issue 34".

https://code.google.com/p/google-api-go-client/issues/detail?id=34

[Brad Fitzpatrick]

On Sun, Jun 2, 2013 at 10:40 PM, Andrew Gerrand <a...@golang.org> wrote:

added "Update issue 34".

https://code.google.com/p/google-api-go-client/issues/detail?id=34

Isn't that exactly what we were skeptical of during the code review?

[Andrew Gerrand]

On 3 June 2013 15:51, Brad Fitzpatrick <brad...@golang.org> wrote:

Isn't that exactly what we were skeptical of during the code review?

Yes. But Francesc reported that it works. Maybe it does work for Storage (the API he was testing), and doesn't work for Drive? I await Francesc's commentary on this.

[a...@golang.org]

*** Submitted as
https://code.google.com/p/google-api-go-client/source/detail?r=5d818df85e09
***

undo CL 9672043 / 38d1b743fd69

I have had reports that this breaks user code.

Update issue 34

««« original CL description
google-api-go-client: update generated code with new param encoding from
https://codereview.appspot.com/9671043/

R=adg, bradfitz
CC=golang-dev
https://codereview.appspot.com/9672043
»»»

R=campoy, bradfitz
CC=golang-dev
https://codereview.appspot.com/9958043


https://codereview.appspot.com/9958043/

[Francesc Campoy Flores]

I tested it with other APIs too and got emails from go nuts members confirming it worked for their code too.

Could you send the report for the error? I'm curious about it.

[Brad Fitzpatrick]

Oh, I bet the issue is Go 1.0.  I recall Opaque changes in Go 1.1 which probably makes this work.

Should we require Go 1.1 for google-api-go-client?

On Wed, Jun 5, 2013 at 8:44 AM, Ikai Lan <ik...@google.com> wrote:

I tried with the API client after the rollback and it worked. I didn't know how to upgrade the API client using "go get"; it didn't seem to do anything, even with the -a flag so I just did an "rm -rf" in the $GOROOT/src/pkg directory.

The report from the error is in the last email I sent you. The issue is that this is the working request (after rollback):

GET /youtube/v3/channels?mine=true&alt=json&part=contentDetails HTTP/1.1

Host: www.googleapis.com

User-Agent: google-api-go-client/0.5

Authorization: Bearer ya29.AHES6ZSJ1rNPH20SQtkud_aY6pNc3Tajf-ivVdquFqflgL7x

Here is what the client was doing pre-rollback:

POST //www.googleapis.com/upload/drive/v2/files?uploadType=multipart&alt=json HTTP/1.1

Host: www.googleapis.com

User-Agent: google-api-go-client/0.5

Content-Length: 23301

Authorization: Bearer ya29.AHES6ZSzpiJ0HuHW-8B86Q96ofG5OoW6W5AMLyiN-HkvMYXo996eAg

The only difference the APIs are different is because I'm writing these really quickly because I don't have my Drive sample open, but the key difference is in the first line. Pre-rollback, the clients were prefixing //www.googleapis.com/ to the URI, which was breaking the Apiary call. I think the issue comes from here:

https://code.google.com/p/google-api-go-client/source/diff?spec=svn5d818df85e09a157dee9e45fb74f8d6527dd3718&r=5d818df85e09a157dee9e45fb74f8d6527dd3718&format=side&path=/youtube/v3/youtube-gen.go

See line 2430. Code that doesn't work:

req.URL.Opaque = "//" + req.URL.Host + req.URL.Path

This appears a few more times in the version that is broken. I'm using Go version 1.0.3 and the version of Goauth recent as of a week ago.

--

Ikai Lan | YouTube Developer Relations

Google New York | 76 Ninth Ave, New York, NY, 10011 | +1 212-565-0000

[Ikai Lan]

I tried with the API client after the rollback and it worked. I didn't know how to upgrade the API client using "go get"; it didn't seem to do anything, even with the -a flag so I just did an "rm -rf" in the $GOROOT/src/pkg directory.

The report from the error is in the last email I sent you. The issue is that this is the working request (after rollback):

GET /youtube/v3/channels?mine=true&alt=json&part=contentDetails HTTP/1.1

Host: www.googleapis.com

User-Agent: google-api-go-client/0.5

Authorization: Bearer ya29.AHES6ZSJ1rNPH20SQtkud_aY6pNc3Tajf-ivVdquFqflgL7x

Here is what the client was doing pre-rollback:

POST //www.googleapis.com/upload/drive/v2/files?uploadType=multipart&alt=json HTTP/1.1

Host: www.googleapis.com

User-Agent: google-api-go-client/0.5

Content-Length: 23301

Authorization: Bearer ya29.AHES6ZSzpiJ0HuHW-8B86Q96ofG5OoW6W5AMLyiN-HkvMYXo996eAg

The only difference the APIs are different is because I'm writing these really quickly because I don't have my Drive sample open, but the key difference is in the first line. Pre-rollback, the clients were prefixing //www.googleapis.com/ to the URI, which was breaking the Apiary call. I think the issue comes from here:

https://code.google.com/p/google-api-go-client/source/diff?spec=svn5d818df85e09a157dee9e45fb74f8d6527dd3718&r=5d818df85e09a157dee9e45fb74f8d6527dd3718&format=side&path=/youtube/v3/youtube-gen.go

See line 2430. Code that doesn't work:

req.URL.Opaque = "//" + req.URL.Host + req.URL.Path

This appears a few more times in the version that is broken. I'm using Go version 1.0.3 and the version of Goauth recent as of a week ago.

--

Ikai Lan | YouTube Developer Relations

Google New York | 76 Ninth Ave, New York, NY, 10011 | +1 212-565-0000

On Tue, Jun 4, 2013 at 8:43 PM, Francesc Campoy Flores <cam...@google.com> wrote:

[Ikai Lan]

It should be your team's call. In general I'm inclined to say no, but as of right now this change will impact zero of our existing developers, and you guys have a better gauge on whether Go developers care or not that they have to upgrade to use Apiary.

--

Ikai Lan | YouTube Developer Relations

Google New York | 76 Ninth Ave, New York, NY, 10011 | +1 212-565-0000

[Andrew Gerrand]

As soon as Go 1.1 is generally available on App Engine, we should require all users to run Go 1.1 to use the google-api-go-client. I would even be inclined to add a log message on startup to that effect.

[Ikai Lan]

SGTM. I can even include a check for Go 1.1 if you're going to put Opaque back in. Is there a better way to check for 1.1 than http://golang.org/pkg/runtime/#Version?

--

Ikai Lan | YouTube Developer Relations

Google New York | 76 Ninth Ave, New York, NY, 10011 | +1 212-565-0000