Google-cloud-ruby: Support ETags in Dataset CRUD

[quartzmo]

CRUD operations on datasets should use the ETag header for optimistic locking. See Using patch in a read-modify-write cycle.

Questions:

• Opt-in or opt-out?
• Raise error on outdated ETag?
• Provide some kind of force overwrite?

[quartzmo]

@blowmage @geigerj @tswast

I have implemented a simple version of this feature that sends the ETag value in the patch request. However, I am not seeing the expected behavior in my acceptance test, which is very similar to the google-cloud-dotnet integration test (which apparently passes.) Here is the test failure that I get both on my dev account and on our shared CI account:

  1) Failure:
Google::Cloud::Bigquery::Dataset::bigquery#test_0003_should fail to set metadata with stale etag [/Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/acceptance/bigquery/dataset_test.rb:110]:
Google::Cloud::Error expected but nothing was raised.

Any ideas?

[tswast]

@jskeet I remember you had a comment on the requirements doc about this. Mind taking a look at the Ruby implementation?

[jskeet]

@tswast: I don't know Ruby so would almost certainly miss details. I'm happy to answer specific questions though.

One thing to be aware of is that the etag in the JSON returned when you create a table/dataset is currently different from the etag you'll get if you immediately fetch the object again.

[quartzmo]

My problem is that no matter what value I send in the etag field of the object in the patch request, I never get an error. (My expectation is that if I send an etag value and it doesn't match the current value in the backend, I should get an error.)

Here is a test run with puts debug logging:

dataset_test.rb

# Modify on the server, which will change the etag
dataset.description = "Description 1"
stale.etag.wont_equal dataset.etag
puts "current etag: #{dataset.etag}"
expect { stale.description = "Description 2" }.must_raise Google::Cloud::Error

dataset.rb

patch_args[:etag] = etag
patch_gapi = Google::Apis::BigqueryV2::Dataset.new patch_args
puts "sending etag: #{patch_gapi.etag}"
@gapi = service.patch_dataset dataset_id, patch_gapi

Test output

current etag: "U174L-uZHiGsoJvSMZ_oqpuY9e8/MTUwMjg5OTA0OTc2Mw"
sending etag: "U174L-uZHiGsoJvSMZ_oqpuY9e8/MTUwMjg5OTA0NzA2Ng"
...
  1) Failure:
Google::Cloud::Bigquery::Dataset::bigquery#test_0003_should fail to set metadata with stale etag [/Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/acceptance/bigquery/dataset_test.rb:110]:
Google::Cloud::Error expected but nothing was raised.

[jskeet]

Ah - just to check, is anything converting that into an If-Match header? (The .NET REST library does this automatically - implementation.)

[quartzmo]

@jskeet Thanks for this crucial bit of info. I bet this is the problem.

[quartzmo]

@jskeet Thanks, setting the If-Match header does the trick, I now get <Google::Apis::ClientError: conditionNotMet: Precondition Failed>. Pretty cool how the .NET REST lib does that.

[blowmage]

One thing to be aware of is that the etag in the JSON returned when you create a table/dataset is currently different from the etag you'll get if you immediately fetch the object again.

@jskeet Wow, really? Why is that? Should we always get the resource after creating it to ensure that users always have a valid etag?

[jskeet]

Unsure. I'm chasing it internally - I'd really like this to be changed server-side.

[blowmage]

One of our requirements is to use etag on changes, so it looks like we will add a get call after create so that the user always has a valid etag. If the server-side implementation changes we can remove the get call.

[quartzmo]

it looks like we will add a get call after create so that the user always has a valid etag.

@blowmage Do you want to request this on PR #1674?

[quartzmo]

@jskeet I am seeing a similar issue with service.patch_table. The table representation returned by service.patch_table does NOT have the updated etag, but rather the stale etag. (But a get after a patch returns a table representation with the new etag.)

Here is some debug logging:

def patch_gapi! *attributes
  return if attributes.empty?
  ensure_service!
  patch_args = Hash[attributes.map do |attr|
    [attr, @gapi.send(attr)]
  end]
  patch_gapi = Google::Apis::BigqueryV2::Table.new patch_args
  patch_gapi.etag = etag if etag
  puts "old: #{etag}"
  @gapi = service.patch_table dataset_id, table_id, patch_gapi
  puts "new: #{etag}"
end

And the output:

# Running:

old: "U174L-uZHiGsoJvSMZ_oqpuY9e8/MTUwMjkxNzAwNTgxNg"
new: "U174L-uZHiGsoJvSMZ_oqpuY9e8/MTUwMjkxNzAwNTgxNg"

This is different from patch_dataset, which works correctly.

[jskeet]

@quartzmo: Hmm - that's very odd. I haven't looked at that in C#. I'll have a look tomorrow.

[jskeet]

Confirmed, I see the same behaviour for both patch and update of tables, but not of datasets. Will pursue that internally too.

[quartzmo]

Closed by #1674.

[quartzmo]

@jskeet. As of yesterday (2017-09-29), it appears that this issue with invalid etags from the service has been resolved, based on a couple of acceptance tests that we put in place to detect a fix:

  2) Failure:
Google::Cloud::Bigquery::Dataset::bigquery#test_0004_etag from get dataset is different from etag returned by Service#insert_dataset [/Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/acceptance/bigquery/dataset_test.rb:133]:
Expected "\"qaujFrjMT9Vbhl4LPdRuJLP1c20/MTUwNDExNDkyMjA2MQ\"" to not be equal to "\"qaujFrjMT9Vbhl4LPdRuJLP1c20/MTUwNDExNDkyMjA2MQ\"".

  4) Failure:
Google::Cloud::Bigquery::Table::bigquery#test_0005_etag from get table is different from etag returned by Service#insert_table [/Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/acceptance/bigquery/table_test.rb:170]:
Expected "\"qaujFrjMT9Vbhl4LPdRuJLP1c20/MTUwNDExNTAwNjEzNA\"" to not be equal to "\"qaujFrjMT9Vbhl4LPdRuJLP1c20/MTUwNDExNTAwNjEzNA\"".

Therefore, I am now preparing a PR to revert our workarounds for this issue.

[jskeet]

Right - I was going to report that good news, but I've been seeing issues with etags being ignored when updating datasets.

[quartzmo]

Oh, thanks for pointing this out. In that case, I'll do a partial revert just for the inserts.

[quartzmo]

@jskeet I'm currently seeing etag failures on Table updates, but not Dataset:

  1) Failure:
Google::Cloud::Bigquery::Table::bigquery#test_0003_should fail to set metadata with stale etag [/Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/acceptance/bigquery/table_test.rb:133]:
Expected "\"qaujFrjMT9Vbhl4LPdRuJLP1c20/MTUwNDExODQ5MjM2Nw\"" to not be equal to "\"qaujFrjMT9Vbhl4LPdRuJLP1c20/MTUwNDExODQ5MjM2Nw\"".

  2) Error:
Google::Cloud::Bigquery::Table::bigquery#test_0006_updates its schema:
Google::Cloud::FailedPreconditionError: conditionNotMet: Precondition Failed
    /Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/lib/google/cloud/bigquery/service.rb:606:in `rescue in execute'
    /Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/lib/google/cloud/bigquery/service.rb:604:in `execute'
    /Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/lib/google/cloud/bigquery/service.rb:161:in `patch_table'
    /Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/lib/google/cloud/bigquery/table.rb:975:in `patch_gapi!'
    /Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/lib/google/cloud/bigquery/table.rb:458:in `schema'
    /Users/quartzmo/code/google/codez/gcloud-ruby/google-cloud-bigquery/acceptance/bigquery/table_test.rb:177:in `block (2 levels) in <top (required)>'

[jskeet]

We may be talking about different things - I'm talking about deliberately providing an incorrect etag in an If-Match header on update, and expecting (but not getting) a failure. Once that's fixed, I'll look at the other issues we've had :)

[quartzmo]

Yes, you're right. Thanks for the clarification.