Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/routes/routeBackbeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -1786,6 +1786,7 @@ const indexEntrySchema = joi.object({
}),
)
.required(),
partialFilterExpression: joi.object(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we validate it ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean declare the schema ? Can be hard as it's mongo but pretty sure we can find some joi lib to validate mongo ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is validated by mongo during the call, so is it worth duplicating the validation?

  • we don't actually own the format, we just pass it through: so not sure we add value.
  • it's fine not to own it, since this is an internal API only.

(the alternative would be to define our own "index definition format", and map it to mongo format in cloudserver : in that case we should indeed do validation)

});

const indexingSchema = joi.array().items(indexEntrySchema).min(1);
Expand Down
98 changes: 98 additions & 0 deletions tests/unit/routes/routeBackbeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -1540,3 +1540,101 @@ describe('routeBackbeat authorization', () => {
});
});
});

describe('routeBackbeat index add payload validation', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing a test : should reject if the field is not well formatted ?

const bucketName = 'bucketname';
let endPromise;
let resolveEnd;
let response;

function makeIndexRequest(payload) {
const body = JSON.stringify(payload);
return new DummyRequest(
{
method: 'POST',
headers: { 'content-length': body.length },
url: `/_/backbeat/index/${bucketName}?operation=add`,
},
body,
);
}

beforeEach(() => {
endPromise = new Promise(resolve => {
resolveEnd = resolve;
});
response = {
setHeader: sinon.stub(),
writeHead: sinon.stub(),
end: sinon.stub().callsFake((body, encoding, callback) => {
resolveEnd();
if (callback) {
callback();
}
}),
};
sinon.stub(auth.server, 'doAuth').yields(
null,
new AuthInfo({
canonicalID: 'abcdef/lifecycle',
accountDisplayName: 'Lifecycle Service Account',
}),
undefined,
undefined,
{},
);
sinon.stub(metadata, 'putBucketIndexes').yields(null);
});

afterEach(() => {
sinon.restore();
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing test where the request fails if backend is not mongo? or is it in some integration tests?

it('should accept indexes with a partialFilterExpression', async () => {
const payload = [
{
keys: [
{ key: 'value.last-modified', order: 1 },
{ key: '_id', order: 1 },
],
name: 'partialIndex',
partialFilterExpression: { _id: { $gte: 'a', $lt: 'b' } },
},
];
routeBackbeat('127.0.0.1', makeIndexRequest(payload), response, log);
void (await endPromise);

assert.strictEqual(response.writeHead.getCall(0).args[0], 200);
assert.deepStrictEqual(metadata.putBucketIndexes.getCall(0).args[1], payload);
});

it('should accept indexes without a partialFilterExpression', async () => {
const payload = [
{
keys: [{ key: '_id', order: 1 }],
name: 'plainIndex',
},
];
routeBackbeat('127.0.0.1', makeIndexRequest(payload), response, log);
void (await endPromise);

assert.strictEqual(response.writeHead.getCall(0).args[0], 200);
assert.deepStrictEqual(metadata.putBucketIndexes.getCall(0).args[1], payload);
});

it('should reject indexes with unknown fields', async () => {
const payload = [
{
keys: [{ key: '_id', order: 1 }],
name: 'badIndex',
unknownField: true,
},
];
routeBackbeat('127.0.0.1', makeIndexRequest(payload), response, log);
void (await endPromise);

const err = JSON.parse(response.end.getCall(0).args[0]);
assert.strictEqual(err.code, 'BadRequest');
assert.strictEqual(metadata.putBucketIndexes.called, false);
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we have "integration" test, where we actually call mongo?
would be nice to have these, and thus test that we reject the call if either keys or partialFilterExpression is not a valid mongo format and that we pass the fields appropriately

Loading