From 714172297220c9800e7770e665df4d28635c5db0 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Tue, 31 Jul 2018 15:29:45 -0400 Subject: [PATCH 1/9] Support Bucket/Object lock operations --- src/bucket.ts | 117 ++++++++++++++++++++++++++++++++++++ src/file.ts | 104 ++++++++++++++++++++++++++++++++ src/index.ts | 11 ++++ system-test/storage.js | 132 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 364 insertions(+) diff --git a/src/bucket.ts b/src/bucket.ts index 10defda2c..f7b58f2f2 100644 --- a/src/bucket.ts +++ b/src/bucket.ts @@ -1586,6 +1586,43 @@ class Bucket extends ServiceObject { }); } + /** + * Lock a previously-defined retention policy. This will prevent changes to + * the policy. + * + * @example + * const storage = require('@google-cloud/storage')(); + * const bucket = storage.bucket('albums'); + * + * bucket.lock(function(err, apiResponse) {}); + * + * //- + * // If the callback is omitted, we'll return a Promise. + * //- + * bucket.lock().then(function(data) { + * const apiResponse = data[0]; + * }); + */ + lock(callback) { + this.getMetadata((err, metadata) => { + if (err) { + callback(err); + return; + } + + this.request( + { + method: 'POST', + uri: '/lockRetentionPolicy', + qs: { + ifMetagenerationMatch: metadata.metageneration, + }, + }, + callback + ); + }); + } + /** * @typedef {array} MakeBucketPrivateResponse * @property {File[]} 0 List of files made private. @@ -1858,6 +1895,34 @@ class Bucket extends ServiceObject { return new Notification(this, id); } + /** + * Remove an already-existing retention policy from this bucket. + * + * @param {SetBucketMetadataCallback} [callback] Callback function. + * @returns {Promise} + * + * @example + * const storage = require('@google-cloud/storage')(); + * const bucket = storage.bucket('albums'); + * + * bucket.removeRetentionPeriod(function(err, apiResponse) {}); + * + * //- + * // If the callback is omitted, we'll return a Promise. + * //- + * bucket.removeRetentionPeriod().then(function(data) { + * const apiResponse = data[0]; + * }); + */ + removeRetentionPeriod(callback) { + this.setMetadata( + { + retentionPolicy: null, + }, + callback + ); + } + /** * Makes request and applies userProject query parameter if necessary. * @@ -1989,6 +2054,13 @@ class Bucket extends ServiceObject { * }, function(err, apiResponse) {}); * * //- + * // Set the default event-based hold value for new objects in this bucket. + * //- + * bucket.setMetadata({ + * defaultEventBasedHold: true + * }, function(err, apiResponse) {}); + * + * //- * // If the callback is omitted, we'll return a Promise. * //- * bucket.setMetadata(metadata).then(function(data) { @@ -2022,6 +2094,51 @@ class Bucket extends ServiceObject { }); } + /** + * Lock all objects contained in the bucket, based on their creation time. Any + * attempt to overwrite or delete objects younger than the retention period + * will result in a `PERMISSION_DENIED` error. + * + * An unlocked retention policy can be modified or removed from the bucket via + * {@link File#removeRetentionPeriod} and {@link File#setRetentionPeriod}. A + * locked retention policy cannot be removed or shortened in duration for the + * lifetime of the bucket. Attempting to remove or decrease period of a locked + * retention policy will result in a `PERMISSION_DENIED` error. + * + * @param {*} duration In seconds, the minimum retention time for all objects + * contained in this bucket. + * @param {SetBucketMetadataCallback} [callback] Callback function. + * @returns {Promise} + * + * @example + * const storage = require('@google-cloud/storage')(); + * const bucket = storage.bucket('albums'); + * + * const DURATION_SECONDS = 15780000; // 6 months. + * + * //- + * // Lock the objects in this bucket for 6 months. + * //- + * bucket.setRetentionPeriod(DURATION_SECONDS, function(err, apiResponse) {}); + * + * //- + * // If the callback is omitted, we'll return a Promise. + * //- + * bucket.setRetentionPeriod(DURATION_SECONDS).then(function(data) { + * const apiResponse = data[0]; + * }); + */ + setRetentionPeriod(duration, callback) { + this.setMetadata( + { + retentionPolicy: { + retentionPeriod: duration, + }, + }, + callback + ); + } + /** * @callback SetStorageClassCallback * @param {?Error} err Request error, if any. diff --git a/src/file.ts b/src/file.ts index 82667916f..6e4345167 100644 --- a/src/file.ts +++ b/src/file.ts @@ -1500,6 +1500,50 @@ class File extends ServiceObject { (this.parent as any).get.call(this, options, callback); } + /** + * @typedef {array} GetExpirationDateResponse + * @property {date} 0 A Date object representing the earliest time this file's + * retention policy will expire. + */ + /** + * @callback GetExpirationDateCallback + * @param {?Error} err Request error, if any. + * @param {date} file A Date object representing the earliest time this file's + * retention policy will expire. + */ + /** + * Get a Date object representing the earliest time this file will expire. + * + * @param {GetExpirationDateCallback} [callback] Callback function. + * @returns {Promise} + * + * @example + * const storage = require('@google-cloud/storage')(); + * const myBucket = storage.bucket('my-bucket'); + * + * const file = myBucket.file('my-file'); + * + * file.getExpirationDate(function(err, expirationDate) { + * // expirationDate is a Date object. + * }); + */ + getExpirationDate(callback) { + this.getMetadata((err, metadata, apiResponse) => { + if (err) { + callback(err, null, apiResponse); + return; + } + + if (!metadata.retentionExpirationTime) { + const error = new Error('An expiration time is not available.'); + callback(error, null, apiResponse); + return; + } + + callback(null, new Date(metadata.retentionExpirationTime), apiResponse); + }); + } + /** * @typedef {array} GetFileMetadataResponse * @property {object} 0 The {@link File} metadata. @@ -1946,6 +1990,41 @@ class File extends ServiceObject { }); } + /** + * Hold this file from its bucket's retention period configuration. + * + * By default, this will set an event-based hold. This is a way to retain + * objects until an event occurs, which is signified by the hold's release + * (i.e. this value is set to false (see @{link File#release})). After being + * released, this object will be subject to bucket-level retention (if any). + * + * Alternatively, you may set a temporary hold. This will follow the same + * behavior as an event-based hold, with the exception that the bucket's + * retention policy will not renew for this file from the time the hold is + * released. + * + * @param {object} [options] Configuration object. + * @param {boolean} [options.temporary=false] - Set a temporary hold. + * @param {SetFileMetadataCallback} [callback] Callback function. + * @returns {Promise} + */ + hold(options, callback) { + if (is.fn(options)) { + callback = options; + options = {}; + } + + const metadata = {}; + + if (options.temporary) { + metadata.temporaryHold = true; + } else { + metadata.eventBasedHold = true; + } + + this.setMetadata(metadata, callback); + } + /** * @typedef {array} MakeFilePrivateResponse * @property {object} 0 The full API response. @@ -2216,6 +2295,31 @@ class File extends ServiceObject { }); } + /** + * Release this file from an event-based or temporary hold. + * + * @param {object} [options] Configuration object. + * @param {boolean} [options.temporary=false] - Release a temporary hold. + * @param {SetFileMetadataCallback} [callback] Callback function. + * @returns {Promise} + */ + release(options, callback) { + if (is.fn(options)) { + callback = options; + options = {}; + } + + const metadata = {}; + + if (options.temporary) { + metadata.temporaryHold = false; + } else { + metadata.eventBasedHold = false; + } + + this.setMetadata(metadata, callback); + } + /** * Makes request and applies userProject query parameter if necessary. * diff --git a/src/index.ts b/src/index.ts index c63213acc..6c73f0739 100644 --- a/src/index.ts +++ b/src/index.ts @@ -361,6 +361,17 @@ class Storage extends Service { * storage.createBucket('new-bucket', metadata, callback); * * //- + * // Create a bucket with a retention policy of 6 months. + * //- + * const metadata = { + * retentionPolicy: { + * retentionPeriod: 15780000 // 6 months in seconds. + * } + * }; + * + * storage.createBucket('new-bucket', metadata, callback); + * + * //- * // Enable versioning on a new bucket. * //- * const metadata = { diff --git a/system-test/storage.js b/system-test/storage.js index 181bc0a9d..c4bd36fd1 100644 --- a/system-test/storage.js +++ b/system-test/storage.js @@ -928,6 +928,138 @@ describe('storage', function() { }); }); + describe('bucket retention policies', function() { + const RETENTION_DURATION_SECONDS = 10; + + describe('bucket', function() { + it('should create a bucket with a retention policy', function() { + const bucket = storage.bucket(generateName()); + + return bucket + .create({ + retentionPolicy: { + retentionPeriod: RETENTION_DURATION_SECONDS, + }, + }) + .then(() => bucket.getMetadata()) + .then(response => { + const metadata = response[0]; + + assert.strictEqual( + metadata.retentionPolicy.retentionPeriod, + `${RETENTION_DURATION_SECONDS}` + ); + }); + }); + + it('should set a retention policy', function() { + const bucket = storage.bucket(generateName()); + + return bucket + .create() + .then(() => bucket.setRetentionPeriod(RETENTION_DURATION_SECONDS)) + .then(() => bucket.getMetadata()) + .then(response => { + const metadata = response[0]; + + assert.strictEqual( + metadata.retentionPolicy.retentionPeriod, + `${RETENTION_DURATION_SECONDS}` + ); + }); + }); + + it('should lock the retention period', function(done) { + const bucket = storage.bucket(generateName()); + + bucket + .create() + .then(() => bucket.setRetentionPeriod(RETENTION_DURATION_SECONDS)) + .then(() => bucket.lock()) + .then(() => bucket.setRetentionPeriod(RETENTION_DURATION_SECONDS / 2)) + .catch(err => { + assert.strictEqual(err.code, 403); + done(); + }); + }); + + it('should remove a retention period', function() { + const bucket = storage.bucket(generateName()); + + return bucket + .create() + .then(() => bucket.setRetentionPeriod(RETENTION_DURATION_SECONDS)) + .then(() => bucket.removeRetentionPeriod()) + .then(() => bucket.getMetadata()) + .then(response => { + const metadata = response[0]; + + assert.strictEqual(metadata.retentionPolicy, undefined); + }); + }); + }); + + describe('file', function() { + const BUCKET = storage.bucket(generateName()); + const FILE = BUCKET.file(generateName()); + + before(function() { + return BUCKET.create({ + retentionPolicy: { + retentionPeriod: 1, + }, + }).then(() => FILE.save('data')); + }); + + afterEach(function() { + return FILE.setMetadata({temporaryHold: null, eventBasedHold: null}); + }); + + after(function() { + return FILE.delete(); + }); + + it('should set and release an event-based hold', function() { + return FILE.hold() + .then(response => { + const metadata = response[0]; + + assert.strictEqual(metadata.eventBasedHold, true); + }) + .then(() => FILE.release()) + .then(() => FILE.getMetadata()) + .then(response => { + const metadata = response[0]; + + assert.strictEqual(metadata.eventBasedHold, false); + }); + }); + + it('should set and release a temporary hold', function() { + return FILE.hold({temporary: true}) + .then(response => { + const metadata = response[0]; + + assert.strictEqual(metadata.temporaryHold, true); + }) + .then(() => FILE.release({temporary: true})) + .then(() => FILE.getMetadata()) + .then(response => { + const metadata = response[0]; + + assert.strictEqual(metadata.temporaryHold, false); + }); + }); + + it('should get an expiration date', function() { + return FILE.getExpirationDate().then(response => { + const expirationDate = response[0]; + assert(expirationDate instanceof Date); + }); + }); + }); + }); + describe('requester pays', function() { const HAS_2ND_PROJECT = is.defined(process.env.GCN_STORAGE_2ND_PROJECT_ID); let bucket; From 95cd619d68a0dc6a1e4c42275d31ec3acf496e81 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Mon, 20 Aug 2018 10:33:13 -0400 Subject: [PATCH 2/9] Doc updates. --- src/bucket.ts | 6 ++++-- src/file.ts | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/bucket.ts b/src/bucket.ts index f7b58f2f2..2b41c7ec0 100644 --- a/src/bucket.ts +++ b/src/bucket.ts @@ -1896,7 +1896,8 @@ class Bucket extends ServiceObject { } /** - * Remove an already-existing retention policy from this bucket. + * Remove an already-existing retention policy from this bucket, if it is not + * locked. * * @param {SetBucketMetadataCallback} [callback] Callback function. * @returns {Promise} @@ -2103,7 +2104,8 @@ class Bucket extends ServiceObject { * {@link File#removeRetentionPeriod} and {@link File#setRetentionPeriod}. A * locked retention policy cannot be removed or shortened in duration for the * lifetime of the bucket. Attempting to remove or decrease period of a locked - * retention policy will result in a `PERMISSION_DENIED` error. + * retention policy will result in a `PERMISSION_DENIED` error. You can still + * increase the policy. * * @param {*} duration In seconds, the minimum retention time for all objects * contained in this bucket. diff --git a/src/file.ts b/src/file.ts index 6e4345167..aca073478 100644 --- a/src/file.ts +++ b/src/file.ts @@ -1512,7 +1512,8 @@ class File extends ServiceObject { * retention policy will expire. */ /** - * Get a Date object representing the earliest time this file will expire. + * If this bucket has a retention policy defined, use this method to get a + * Date object representing the earliest time this file will expire. * * @param {GetExpirationDateCallback} [callback] Callback function. * @returns {Promise} From 77961362acc02cb2b7372875a505330ceb4a147b Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 6 Sep 2018 13:31:48 -0400 Subject: [PATCH 3/9] Remove File#hold(), File#release() and replace with docs in File#setMetadata(). --- src/file.ts | 78 +++++++++++++---------------------------------------- 1 file changed, 18 insertions(+), 60 deletions(-) diff --git a/src/file.ts b/src/file.ts index aca073478..7b6ef738f 100644 --- a/src/file.ts +++ b/src/file.ts @@ -1991,41 +1991,6 @@ class File extends ServiceObject { }); } - /** - * Hold this file from its bucket's retention period configuration. - * - * By default, this will set an event-based hold. This is a way to retain - * objects until an event occurs, which is signified by the hold's release - * (i.e. this value is set to false (see @{link File#release})). After being - * released, this object will be subject to bucket-level retention (if any). - * - * Alternatively, you may set a temporary hold. This will follow the same - * behavior as an event-based hold, with the exception that the bucket's - * retention policy will not renew for this file from the time the hold is - * released. - * - * @param {object} [options] Configuration object. - * @param {boolean} [options.temporary=false] - Set a temporary hold. - * @param {SetFileMetadataCallback} [callback] Callback function. - * @returns {Promise} - */ - hold(options, callback) { - if (is.fn(options)) { - callback = options; - options = {}; - } - - const metadata = {}; - - if (options.temporary) { - metadata.temporaryHold = true; - } else { - metadata.eventBasedHold = true; - } - - this.setMetadata(metadata, callback); - } - /** * @typedef {array} MakeFilePrivateResponse * @property {object} 0 The full API response. @@ -2296,31 +2261,6 @@ class File extends ServiceObject { }); } - /** - * Release this file from an event-based or temporary hold. - * - * @param {object} [options] Configuration object. - * @param {boolean} [options.temporary=false] - Release a temporary hold. - * @param {SetFileMetadataCallback} [callback] Callback function. - * @returns {Promise} - */ - release(options, callback) { - if (is.fn(options)) { - callback = options; - options = {}; - } - - const metadata = {}; - - if (options.temporary) { - metadata.temporaryHold = false; - } else { - metadata.eventBasedHold = false; - } - - this.setMetadata(metadata, callback); - } - /** * Makes request and applies userProject query parameter if necessary. * @@ -2482,6 +2422,24 @@ class File extends ServiceObject { * }); * * //- + * // Set a temporary hold on this file from its bucket's retention period + * // configuration. + * // + * file.setMetadata({ + * temporaryHold: true + * }, function(err, apiResponse) {}); + * + * //- + * // Alternatively, you may set a temporary hold. This will follow the same + * // behavior as an event-based hold, with the exception that the bucket's + * // retention policy will not renew for this file from the time the hold is + * // released. + * //- + * file.setMetadata({ + * eventBasedHold: true + * }, function(err, apiResponse) {}); + * + * //- * // If the callback is omitted, we'll return a Promise. * //- * file.setMetadata(metadata).then(function(data) { From 06d0e895f3ed7a96d9c391b2ea875971f5fed1b4 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 6 Sep 2018 14:43:30 -0400 Subject: [PATCH 4/9] Add tests. --- src/bucket.ts | 9 +++-- src/file.ts | 4 +-- system-test/storage.js | 8 ++--- test/bucket.ts | 82 ++++++++++++++++++++++++++++++++++++++++++ test/file.ts | 60 +++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 9 deletions(-) diff --git a/src/bucket.ts b/src/bucket.ts index 2b41c7ec0..58caa4e8e 100644 --- a/src/bucket.ts +++ b/src/bucket.ts @@ -1455,7 +1455,7 @@ class Bucket extends ServiceObject { /** * @callback GetBucketMetadataCallback * @param {?Error} err Request error, if any. - * @param {object} files The bucket metadata. + * @param {object} metadata The bucket metadata. * @param {object} apiResponse The full API response. */ /** @@ -1590,6 +1590,9 @@ class Bucket extends ServiceObject { * Lock a previously-defined retention policy. This will prevent changes to * the policy. * + * @param {SetBucketMetadataCallback} [callback] Callback function. + * @returns {Promise} + * * @example * const storage = require('@google-cloud/storage')(); * const bucket = storage.bucket('albums'); @@ -1604,9 +1607,9 @@ class Bucket extends ServiceObject { * }); */ lock(callback) { - this.getMetadata((err, metadata) => { + this.getMetadata((err, metadata, apiResponse) => { if (err) { - callback(err); + callback(err, null, apiResponse); return; } diff --git a/src/file.ts b/src/file.ts index 7b6ef738f..0169406e0 100644 --- a/src/file.ts +++ b/src/file.ts @@ -1508,8 +1508,8 @@ class File extends ServiceObject { /** * @callback GetExpirationDateCallback * @param {?Error} err Request error, if any. - * @param {date} file A Date object representing the earliest time this file's - * retention policy will expire. + * @param {date} expirationDate A Date object representing the earliest time + * this file's retention policy will expire. */ /** * If this bucket has a retention policy defined, use this method to get a diff --git a/system-test/storage.js b/system-test/storage.js index c4bd36fd1..19ff4cb77 100644 --- a/system-test/storage.js +++ b/system-test/storage.js @@ -1020,13 +1020,13 @@ describe('storage', function() { }); it('should set and release an event-based hold', function() { - return FILE.hold() + return FILE.setMetadata({eventBasedHold: true}) .then(response => { const metadata = response[0]; assert.strictEqual(metadata.eventBasedHold, true); }) - .then(() => FILE.release()) + .then(() => FILE.setMetadata({eventBasedHold: false})) .then(() => FILE.getMetadata()) .then(response => { const metadata = response[0]; @@ -1036,13 +1036,13 @@ describe('storage', function() { }); it('should set and release a temporary hold', function() { - return FILE.hold({temporary: true}) + return FILE.setMetadata({temporaryHold: true}) .then(response => { const metadata = response[0]; assert.strictEqual(metadata.temporaryHold, true); }) - .then(() => FILE.release({temporary: true})) + .then(() => FILE.setMetadata({temporaryHold: false})) .then(() => FILE.getMetadata()) .then(response => { const metadata = response[0]; diff --git a/test/bucket.ts b/test/bucket.ts index 1564ef04f..6753d652c 100644 --- a/test/bucket.ts +++ b/test/bucket.ts @@ -1558,6 +1558,56 @@ describe('Bucket', () => { }); }); + describe('lock', () => { + it('should refresh metadata', done => { + bucket.getMetadata = () => { + done(); + }; + + bucket.lock(assert.ifError); + }); + + it('should return error from getMetadata', done => { + const error = new Error('Error.'); + const apiResponse = {} + + bucket.getMetadata = callback => { + callback(error, null, apiResponse); + }; + + bucket.lock((err, metadata, apiResponse_) => { + assert.strictEqual(err, error); + assert.strictEqual(metadata, null); + assert.strictEqual(apiResponse_, apiResponse); + done(); + }); + }); + + it('should make the correct request', done => { + const metadata = { + metageneration: 8, + }; + + bucket.getMetadata = callback => { + callback(null, metadata); + }; + + bucket.request = (reqOpts, callback) => { + assert.deepStrictEqual(reqOpts, { + method: 'POST', + uri: '/lockRetentionPolicy', + qs: { + ifMetagenerationMatch: metadata.metageneration, + }, + }); + + callback(); // done() + }; + + bucket.lock(done); + }); + }); + describe('makePrivate', () => { it('should set predefinedAcl & privatize files', done => { let didSetPredefinedAcl = false; @@ -1719,6 +1769,20 @@ describe('Bucket', () => { }); }); + describe('removeRetentionPeriod', () => { + it('should call setMetadata correctly', done => { + bucket.setMetadata = (metadata, callback) => { + assert.deepStrictEqual(metadata, { + retentionPolicy: null, + }); + + callback(); // done() + }; + + bucket.removeRetentionPeriod(done); + }); + }); + describe('request', () => { const USER_PROJECT = 'grape-spaceship-123'; @@ -1886,6 +1950,24 @@ describe('Bucket', () => { }); }); + describe('setRetentionPeriod', () => { + it('should call setMetadata correctly', done => { + const duration = 90000; + + bucket.setMetadata = (metadata, callback) => { + assert.deepStrictEqual(metadata, { + retentionPolicy: { + retentionPeriod: duration, + }, + }); + + callback(); // done() + }; + + bucket.setRetentionPeriod(duration, done); + }); + }); + describe('setStorageClass', () => { const STORAGE_CLASS = 'NEW_STORAGE_CLASS'; const OPTIONS = {}; diff --git a/test/file.ts b/test/file.ts index c9c7ad5d7..3754f0816 100644 --- a/test/file.ts +++ b/test/file.ts @@ -1966,6 +1966,66 @@ describe('File', () => { }); }); + describe('getExpirationDate', () => { + it('should refresh metadata', done => { + file.getMetadata = () => { + done(); + }; + + file.getExpirationDate(assert.ifError); + }); + + it('should return error from getMetadata', done => { + const error = new Error('Error.'); + const apiResponse = {}; + + file.getMetadata = callback => { + callback(error, null, apiResponse); + }; + + file.getExpirationDate((err, expirationDate, apiResponse_) => { + assert.strictEqual(err, error); + assert.strictEqual(expirationDate, null); + assert.strictEqual(apiResponse_, apiResponse); + done(); + }); + }); + + it('should return an error if there is no expiration time', done => { + const apiResponse = {}; + + file.getMetadata = callback => { + callback(null, {}, apiResponse); + }; + + file.getExpirationDate((err, expirationDate, apiResponse_) => { + assert.strictEqual(err.message, `An expiration time is not available.`); + assert.strictEqual(expirationDate, null); + assert.strictEqual(apiResponse_, apiResponse); + done(); + }); + }); + + it('should return the expiration time as a Date object', done => { + const expirationTime = new Date(); + + const apiResponse = { + retentionExpirationTime: expirationTime.toJSON(), + }; + + file.getMetadata = callback => { + callback(null, apiResponse, apiResponse); + }; + + file.getExpirationDate((err, expirationDate, apiResponse_) => { + assert.ifError(err); + assert.deepStrictEqual(expirationDate, expirationTime); + assert.strictEqual(apiResponse_, apiResponse); + done(); + }); + }); + }); + describe('getMetadata', () => { it('should make the correct request', done => { extend(file.parent, { From b54ac3a9070470fefb0a0186cafeaafd4e06970b Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 6 Sep 2018 14:53:28 -0400 Subject: [PATCH 5/9] fixed. --- src/bucket.ts | 35 ++++++++++++++++------------------- test/bucket.ts | 8 ++++---- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/bucket.ts b/src/bucket.ts index 58caa4e8e..dc3f59aeb 100644 --- a/src/bucket.ts +++ b/src/bucket.ts @@ -1614,15 +1614,14 @@ class Bucket extends ServiceObject { } this.request( - { - method: 'POST', - uri: '/lockRetentionPolicy', - qs: { - ifMetagenerationMatch: metadata.metageneration, + { + method: 'POST', + uri: '/lockRetentionPolicy', + qs: { + ifMetagenerationMatch: metadata.metageneration, + }, }, - }, - callback - ); + callback); }); } @@ -1920,11 +1919,10 @@ class Bucket extends ServiceObject { */ removeRetentionPeriod(callback) { this.setMetadata( - { - retentionPolicy: null, - }, - callback - ); + { + retentionPolicy: null, + }, + callback); } /** @@ -2135,13 +2133,12 @@ class Bucket extends ServiceObject { */ setRetentionPeriod(duration, callback) { this.setMetadata( - { - retentionPolicy: { - retentionPeriod: duration, + { + retentionPolicy: { + retentionPeriod: duration, + }, }, - }, - callback - ); + callback); } /** diff --git a/test/bucket.ts b/test/bucket.ts index 6753d652c..51a919e91 100644 --- a/test/bucket.ts +++ b/test/bucket.ts @@ -1569,7 +1569,7 @@ describe('Bucket', () => { it('should return error from getMetadata', done => { const error = new Error('Error.'); - const apiResponse = {} + const apiResponse = {}; bucket.getMetadata = callback => { callback(error, null, apiResponse); @@ -1601,7 +1601,7 @@ describe('Bucket', () => { }, }); - callback(); // done() + callback(); // done() }; bucket.lock(done); @@ -1776,7 +1776,7 @@ describe('Bucket', () => { retentionPolicy: null, }); - callback(); // done() + callback(); // done() }; bucket.removeRetentionPeriod(done); @@ -1961,7 +1961,7 @@ describe('Bucket', () => { }, }); - callback(); // done() + callback(); // done() }; bucket.setRetentionPeriod(duration, done); From 30a68c25e437f54eabc7e26b74207f039ac3758b Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 6 Sep 2018 15:06:48 -0400 Subject: [PATCH 6/9] add system tests to verify hold behavior. --- system-test/storage.js | 70 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/system-test/storage.js b/system-test/storage.js index 19ff4cb77..1be59aa5c 100644 --- a/system-test/storage.js +++ b/system-test/storage.js @@ -1058,6 +1058,76 @@ describe('storage', function() { }); }); }); + + describe('operations on held objects', function() { + const BUCKET = storage.bucket(generateName()); + const FILES = []; + + const RETENTION_PERIOD_SECONDS = 5; // Each test has this much time! + + function createFile(callback) { + const file = BUCKET.file(generateName()); + FILES.push(file); + + file.save('data', function(err) { + if (err) { + callback(err); + return; + } + + callback(null, file); + }); + } + + function deleteFiles(callback) { + async.each(FILES, function(file, next) { + file.setMetadata({temporaryHold: null}, function(err) { + if (err) { + next(err); + return; + } + + file.delete(next); + }); + }, callback); + } + + before(function() { + return BUCKET.create({ + retentionPolicy: { + retentionPeriod: RETENTION_PERIOD_SECONDS, + }, + }); + }); + + after(function(done) { + setTimeout(deleteFiles, RETENTION_PERIOD_SECONDS * 1000, done); + }); + + //verify how the client library behaves when holds are enabled + //and attempting to perform an overwrite and delete. + it('should block an overwrite request', function(done) { + createFile(function(err, file) { + assert.ifError(err); + + file.save('new data', function(err) { + assert.strictEqual(err.code, 403); + done(); + }); + }); + }); + + it('should block a delete request', function(done) { + createFile(function(err, file) { + assert.ifError(err); + + file.delete(function(err) { + assert.strictEqual(err.code, 403); + done(); + }); + }); + }); + }); }); describe('requester pays', function() { From 8b5492d1a452164060d6ac050516e7f9f5268085 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 7 Sep 2018 08:38:46 -0400 Subject: [PATCH 7/9] Require metageneration to bucket#lock() --- src/bucket.ts | 31 ++++++++++++++++--------------- test/bucket.ts | 38 ++++++++------------------------------ 2 files changed, 24 insertions(+), 45 deletions(-) diff --git a/src/bucket.ts b/src/bucket.ts index dc3f59aeb..5ef7f0ff7 100644 --- a/src/bucket.ts +++ b/src/bucket.ts @@ -1590,6 +1590,10 @@ class Bucket extends ServiceObject { * Lock a previously-defined retention policy. This will prevent changes to * the policy. * + * @throws {Error} if a metageneration is not provided. + * + * @param {Number|String} metageneration The bucket's metageneration. This is + * accesssible from calling {@link File#getMetadata}. * @param {SetBucketMetadataCallback} [callback] Callback function. * @returns {Promise} * @@ -1606,23 +1610,20 @@ class Bucket extends ServiceObject { * const apiResponse = data[0]; * }); */ - lock(callback) { - this.getMetadata((err, metadata, apiResponse) => { - if (err) { - callback(err, null, apiResponse); - return; - } + lock(metageneration, callback) { + if (!is.number(metageneration) && !is.string(metageneration)) { + throw new Error('A metageneration must be provided.'); + } - this.request( - { - method: 'POST', - uri: '/lockRetentionPolicy', - qs: { - ifMetagenerationMatch: metadata.metageneration, - }, + this.request( + { + method: 'POST', + uri: '/lockRetentionPolicy', + qs: { + ifMetagenerationMatch: metageneration, }, - callback); - }); + }, + callback); } /** diff --git a/test/bucket.ts b/test/bucket.ts index 51a919e91..b06ceb72d 100644 --- a/test/bucket.ts +++ b/test/bucket.ts @@ -1559,52 +1559,30 @@ describe('Bucket', () => { }); describe('lock', () => { - it('should refresh metadata', done => { - bucket.getMetadata = () => { - done(); - }; - - bucket.lock(assert.ifError); - }); - - it('should return error from getMetadata', done => { - const error = new Error('Error.'); - const apiResponse = {}; - - bucket.getMetadata = callback => { - callback(error, null, apiResponse); - }; + it('should throw if a metageneration is not provided', () => { + const expectedError = new RegExp('A metageneration must be provided.'); - bucket.lock((err, metadata, apiResponse_) => { - assert.strictEqual(err, error); - assert.strictEqual(metadata, null); - assert.strictEqual(apiResponse_, apiResponse); - done(); - }); + assert.throws(() => { + bucket.lock(assert.ifError); + }, expectedError); }); it('should make the correct request', done => { - const metadata = { - metageneration: 8, - }; - - bucket.getMetadata = callback => { - callback(null, metadata); - }; + const metageneration = 8; bucket.request = (reqOpts, callback) => { assert.deepStrictEqual(reqOpts, { method: 'POST', uri: '/lockRetentionPolicy', qs: { - ifMetagenerationMatch: metadata.metageneration, + ifMetagenerationMatch: metageneration, }, }); callback(); // done() }; - bucket.lock(done); + bucket.lock(metageneration, done); }); }); From 9c0668b415a360319a7da62e7824bdc0229a69da Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 7 Sep 2018 09:15:05 -0400 Subject: [PATCH 8/9] Lint fix --- system-test/storage.js | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/system-test/storage.js b/system-test/storage.js index 1be59aa5c..6c957c802 100644 --- a/system-test/storage.js +++ b/system-test/storage.js @@ -1080,16 +1080,20 @@ describe('storage', function() { } function deleteFiles(callback) { - async.each(FILES, function(file, next) { - file.setMetadata({temporaryHold: null}, function(err) { - if (err) { - next(err); - return; - } + async.each( + FILES, + function(file, next) { + file.setMetadata({temporaryHold: null}, function(err) { + if (err) { + next(err); + return; + } - file.delete(next); - }); - }, callback); + file.delete(next); + }); + }, + callback + ); } before(function() { From 7050a4d6cb7124002af8211c86a185dc2c6237ec Mon Sep 17 00:00:00 2001 From: Stephen Date: Mon, 10 Sep 2018 12:47:46 -0400 Subject: [PATCH 9/9] Show `metageneration` usage. --- src/bucket.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/bucket.ts b/src/bucket.ts index 5ef7f0ff7..61769be45 100644 --- a/src/bucket.ts +++ b/src/bucket.ts @@ -1601,12 +1601,14 @@ class Bucket extends ServiceObject { * const storage = require('@google-cloud/storage')(); * const bucket = storage.bucket('albums'); * - * bucket.lock(function(err, apiResponse) {}); + * const metageneration = 2; + * + * bucket.lock(metageneration, function(err, apiResponse) {}); * * //- * // If the callback is omitted, we'll return a Promise. * //- - * bucket.lock().then(function(data) { + * bucket.lock(metageneration).then(function(data) { * const apiResponse = data[0]; * }); */