Skip to content

Commit

Permalink
Merge pull request #67 from scimmyjs/fix/listresponse-sortby-multivalue
Browse files Browse the repository at this point in the history 
Fix comparison of multivalue attributes targeted by sortBy parameter in `SCIMMY.Messages.ListResponse` constructor
  • Loading branch information
@sleelin
sleelin authored Jan 15, 2025
2 parents 580d2e4 + 4038071 commit f8a3416
Show file tree
Hide file tree
Showing 3 changed files with 170 additions and 62 deletions.
20 changes: 15 additions & 5 deletions src/lib/messages/listresponse.js
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
/**
* SCIM List Response Message
* @alias SCIMMY.Messages.ListResponse
* @template {SCIMMY.Types.Schema} [T=*] - type of schema instance that will be passed to handlers
* @summary
* * Formats supplied service provider resources as [ListResponse messages](https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2), handling pagination and sort when required.
* @description
* The `ListResponse` class creates a SCIM [ListResponse message](https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2) for a supplied set of resources, and automates pagination and sorting of the included set.
* It is used internally by the `read()` method in each of {@link SCIMMY.Resources|SCIMMY's Resource classes} when requesting a list of resources matching a given query,
* and in the {@link SCIMMY.Messages.SearchRequest#apply|`apply()`} instance method of the {@link SCIMMY.Messages.SearchRequest|SearchRequest} message class.
*
* > **Tip:**
* > When using the {@link SCIMMY.Resources.User|`User`} and {@link SCIMMY.Resources.Group|`Group`} resource classes, their instance `read()` methods will wrap the set of matching query results in a `ListResponse` instance.
* > This happens automatically when retrieving multiple resources, meaning their {@link SCIMMY.Types.Resource~EgressHandler|egress handlers} need only return the array of matching resources, not a `ListResponse` instance of their own.
*/
export class ListResponse {
/**
Expand All @@ -23,11 +32,11 @@ export class ListResponse {

/**
* Instantiate a new SCIM List Response Message with relevant details
* @param {Object|SCIMMY.Types.Schema[]} request - contents of the ListResponse message, or items to include in the list response
* @param {SCIMMY.Messages.ListResponse<T>|T[]} request - contents of the ListResponse message, or items to include in the list response
* @param {SCIMMY.Messages.ListResponse~ListConstraints} [params] - parameters for the list response (i.e. sort details, start index, and items per page)
* @param {Number} [params.count=20] - alias property for itemsPerPage, used only if itemsPerPage is unset
* @param {Number} [params.itemsPerPage=20] - maximum number of items returned in this list response
* @property {Array<Object|SCIMMY.Types.Schema>} Resources - resources included in the list response
* @property {Array<T>} Resources - resources included in the list response
* @property {Number} totalResults - the total number of resources matching a given request
* @property {Number} startIndex - index within total results that included resources start from
* @property {Number} itemsPerPage - maximum number of items returned in this list response
Expand All @@ -36,7 +45,7 @@ export class ListResponse {
const outbound = Array.isArray(request);
const resources = (outbound ? request : request?.Resources ?? []);
const totalResults = (outbound ? resources.totalResults ?? resources.length : request.totalResults);
const {sortBy, sortOrder = "ascending"} = params ?? {};
const {sortBy, sortOrder = "ascending"} = (outbound ? params : {});
const {startIndex = 1, count = 20, itemsPerPage = count} = (outbound ? params : request);

// Verify the ListResponse contents are valid
Expand Down Expand Up @@ -70,8 +79,9 @@ export class ListResponse {
// Do the sort!
this.Resources = this.Resources.sort((a, b) => {
// Resolve target sort values for each side of the comparison (either the "primary" entry, or first entry, in a multi-valued attribute, or the target value)
const ta = paths.reduce((res = {}, path = "") => ((!Array.isArray(res[path]) ? res[path] : (res[path].find(v => !!v.primary) ?? res[0])?.value) ?? ""), a);
const tb = paths.reduce((res = {}, path = "") => ((!Array.isArray(res[path]) ? res[path] : (res[path].find(v => !!v.primary) ?? res[0])?.value) ?? ""), b);
const reducer = (res = {}, path = "") => ((!Array.isArray(res[path]) ? res[path] : (res[path].find(v => !!v.primary) ?? res[path][0])?.value ?? res[path][0]));
const ta = paths.reduce(reducer, a);
const tb = paths.reduce(reducer, b);
const list = [ta, tb];

// If some or all of the targets are unspecified, sort specified value above unspecified value
Expand Down
172 changes: 131 additions & 41 deletions test/lib/messages/listresponse.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,11 @@ describe("SCIMMY.Messages.ListResponse", () => {
});

it("should ignore 'sortOrder' parameter if 'sortBy' parameter is not defined", () => {
assert.doesNotThrow(() => new ListResponse([], {sortOrder: "a string"}),
"ListResponse did not ignore invalid 'sortOrder' parameter when 'sortBy' parameter was not defined");
try {
new ListResponse([], {sortOrder: "a string"});
} catch (ex) {
assert.fail(`ListResponse did not ignore invalid 'sortOrder' parameter when 'sortBy' parameter was not defined\r\n[cause]: ${ex}`);
}
});

it("should expect 'sortOrder' parameter to be either 'ascending' or 'descending' if 'sortBy' parameter is defined", () => {
Expand Down Expand Up @@ -96,14 +99,42 @@ describe("SCIMMY.Messages.ListResponse", () => {
"Instance member 'Resources' was not an array");
});

it("should not include more resources than 'itemsPerPage' parameter", async () => {
it("should equal 'Resources' value included in inbound messages", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.deepStrictEqual(new ListResponse(fixture, {sortBy: "id", sortOrder: "descending"}).Resources, fixture.Resources,
`Instance member 'Resources' did not equal 'Resources' value included in inbound fixture #${suite.indexOf(fixture) + 1}`);
}
});

it("should not include more resources than 'itemsPerPage' value included in inbound messages", async () => {
const {outbound: {source}} = await fixtures;
const fixture = {...template, Resources: source};

for (let itemsPerPage of [2, 5, 10, 200, 1]) {
assert.ok(new ListResponse({...fixture, itemsPerPage}).Resources.length <= itemsPerPage,
"Instance member 'Resources' included more resources than specified in 'itemsPerPage' value of inbound message");
}
});

it("should not include more resources than 'itemsPerPage' parameter when preparing outbound messages", async () => {
const {outbound: {source}} = await fixtures;

for (let length of [2, 5, 10, 200, 1]) {
assert.ok((new ListResponse(source, {itemsPerPage: length})).Resources.length <= length,
"Instance member 'Resources' included more resources than specified in 'itemsPerPage' parameter");
for (let itemsPerPage of [2, 5, 10, 200, 1]) {
assert.ok(new ListResponse(source, {itemsPerPage}).Resources.length <= itemsPerPage,
"Instance member 'Resources' included more resources than specified in 'itemsPerPage' parameter when preparing outbound messages");
}
});

it("should correctly compare Date instances at 'sortBy' path when preparing outbound messages", async () => {
const {outbound: {source, targets: {sortBy: suite}}} = await fixtures;
const {expected, sortBy} = suite.find(({sortBy}) => sortBy === "date");
const actual = new ListResponse(source.map(({id, date}) => ({id, date: new Date(date)})), {sortBy});

assert.deepStrictEqual(actual.Resources.map(({id}) => id), expected,
"ListResponse did not correctly compare Date instances at 'sortBy' path when preparing outbound message");
});
});

describe("#startIndex", () => {
Expand All @@ -121,36 +152,46 @@ describe("SCIMMY.Messages.ListResponse", () => {
"Instance member 'startIndex' was not a positive integer");
});

context("when parsing inbound messages", () => {
it("should equal 'startIndex' value included in message", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.strictEqual((new ListResponse(fixture, {startIndex: 20})).startIndex, fixture.startIndex,
`Instance member 'startIndex' did not equal 'startIndex' value included in inbound fixture #${suite.indexOf(fixture) + 1}`);
}
});
it("should equal 'startIndex' value included in inbound message", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.strictEqual(new ListResponse(fixture, {startIndex: 20}).startIndex, fixture.startIndex,
`Instance member 'startIndex' did not equal 'startIndex' value included in inbound fixture #${suite.indexOf(fixture) + 1}`);
}
});

context("when preparing outbound messages", () => {
it("should be honoured if 'totalResults' is less than 'itemsPerPage'", async () => {
const {outbound: {source}} = await fixtures;

assert.strictEqual(new ListResponse(source, {startIndex: source.length}).Resources.length, 1,
"ListResponse did not honour 'startIndex' when 'totalResults' was less than 'itemsPerPage'");
});
it("should be honoured if 'totalResults' is less than 'itemsPerPage' when preparing outbound messages", async () => {
const {outbound: {source}} = await fixtures;

it("should be honoured if results are not already paginated", async () => {
const {outbound: {source, targets: {startIndex: suite}}} = await fixtures;
assert.strictEqual(new ListResponse(source, {startIndex: source.length}).Resources.length, 1,
"ListResponse did not honour 'startIndex' when 'totalResults' was less than 'itemsPerPage'");
});

it("should be honoured if results are not already paginated when preparing outbound messages", async () => {
const {outbound: {source, targets: {startIndex: suite}}} = await fixtures;

for (let fixture of suite) {
const {expected, length = source.length, sourceRange: [from = 1, to = length] = [], startIndex, itemsPerPage} = fixture;
const actual = new ListResponse(Object.assign(source.slice(from-1, to), {length}), {startIndex, itemsPerPage});

for (let fixture of suite) {
const {expected, length = source.length, sourceRange: [from = 1, to = length] = [], startIndex, itemsPerPage} = fixture;
const actual = new ListResponse(Object.assign(source.slice(from-1, to), {length}), {startIndex, itemsPerPage});

assert.deepStrictEqual(actual.Resources.map(({id}) => id), expected,
`ListResponse startIndex outbound target #${suite.indexOf(fixture)+1} did not honour 'startIndex' value '${startIndex}'`);
}
});
assert.deepStrictEqual(actual.Resources.map(({id}) => id), expected,
`ListResponse startIndex outbound target #${suite.indexOf(fixture)+1} did not honour 'startIndex' value '${startIndex}'`);
}
});

it("should be constrained to a minimum value of one when parsing inbound messages", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.strictEqual(new ListResponse({...fixture, startIndex: -10}).startIndex, 1,
"Instance member 'startIndex' was not constrained to minimum value one when parsing inbound message");
}
});

it("should be constrained to a minimum value of one when preparing outbound messages", () => {
assert.strictEqual(new ListResponse([], {startIndex: -10}).startIndex, 1,
"Instance member 'startIndex' was not constrained to minimum value one when preparing outbound message");
});
});

Expand All @@ -160,23 +201,57 @@ describe("SCIMMY.Messages.ListResponse", () => {
"Instance member 'itemsPerPage' was not defined");
});

it("should be a positive integer", () => {
it("should be a non-negative integer", () => {
const list = new ListResponse();

assert.ok(typeof list.itemsPerPage === "number" && !Number.isNaN(list.itemsPerPage),
"Instance member 'itemsPerPage' was not a number");
assert.ok(list.itemsPerPage > 0 && Number.isInteger(list.itemsPerPage),
"Instance member 'itemsPerPage' was not a positive integer");
assert.ok(list.itemsPerPage >= 0 && Number.isInteger(list.itemsPerPage),
"Instance member 'itemsPerPage' was not a non-negative integer");
});

it("should equal 'itemsPerPage' value included in inbound requests", async () => {
it("should equal 'itemsPerPage' value included in inbound messages", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.strictEqual((new ListResponse(fixture, {itemsPerPage: 200})).itemsPerPage, fixture.itemsPerPage,
assert.strictEqual(new ListResponse(fixture, {itemsPerPage: 200}).itemsPerPage, fixture.itemsPerPage,
`Instance member 'itemsPerPage' did not equal 'itemsPerPage' value included in inbound fixture #${suite.indexOf(fixture) + 1}`);
}
});

it("should equal 'itemsPerPage' parameter specified at instantiation when preparing outbound messages", () => {
assert.strictEqual(new ListResponse([], {itemsPerPage: 1}).itemsPerPage, 1,
"Instance member 'itemsPerPage' did not equal 'itemsPerPage' parameter specified at instantiation when preparing outbound messages");
});

it("should equal 'count' parameter when 'itemsPerPage' parameter is not specified at instantiation", () => {
assert.strictEqual(new ListResponse([], {count: 1}).itemsPerPage, 1,
"Instance member 'itemsPerPage' did not equal 'count' parameter when 'itemsPerPage' parameter was not specified");
});

it("should prefer 'itemsPerPage' parameter over 'count' parameter specified at instantiation", () => {
assert.strictEqual(new ListResponse([], {count: 10, itemsPerPage: 1}).itemsPerPage, 1,
"Instance member 'itemsPerPage' did not prefer 'itemsPerPage' parameter over 'count' parameter specified at instantiation");
});

it("should fall back to the default value when 'itemsPerPage' and 'count' parameters are not specified at instantiation", () => {
assert.strictEqual(new ListResponse([]).itemsPerPage, 20,
"Instance member 'itemsPerPage' did not fall back to default value when 'itemsPerPage' and 'count' parameters were not specified");
});

it("should be constrained to a minimum value of zero when parsing inbound messages", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.strictEqual(new ListResponse({...fixture, itemsPerPage: -10}).itemsPerPage, 0,
"Instance member 'itemsPerPage' was not constrained to minimum value zero when parsing inbound message");
}
});

it("should be constrained to a minimum value of zero when preparing outbound messages", () => {
assert.strictEqual(new ListResponse([], {itemsPerPage: -1}).itemsPerPage, 0,
"Instance member 'itemsPerPage' was not constrained to minimum value zero when preparing outbound message");
});
});

describe("#totalResults", () => {
Expand All @@ -185,22 +260,37 @@ describe("SCIMMY.Messages.ListResponse", () => {
"Instance member 'totalResults' was not defined");
});

it("should be a positive integer", () => {
it("should be a non-negative integer", () => {
const list = new ListResponse();

assert.ok(typeof list.totalResults === "number" && !Number.isNaN(list.totalResults),
"Instance member 'totalResults' was not a number");
assert.ok(list.totalResults >= 0 && Number.isInteger(list.totalResults),
"Instance member 'totalResults' was not a positive integer");
"Instance member 'totalResults' was not a non-negative integer");
});

it("should equal 'totalResults' value included in inbound requests", async () => {
it("should equal 'totalResults' value included in inbound messages", async () => {
const {inbound: suite} = await fixtures;

for (let fixture of suite) {
assert.strictEqual((new ListResponse(fixture, {totalResults: 200})).totalResults, fixture.totalResults,
assert.strictEqual(new ListResponse(fixture, {totalResults: 200}).totalResults, fixture.totalResults,
`Instance member 'totalResults' did not equal 'totalResults' value included in inbound fixture #${suite.indexOf(fixture) + 1}`);
}
});

it("should equal 'totalResults' property of resources array when preparing outbound messages", () => {
assert.strictEqual(new ListResponse(Object.assign([], {totalResults: 100})).totalResults, 100,
"Instance member 'totalResults' did not equal 'totalResults' property of resources array when preparing outbound messages");
});

it("should equal 'length' property of resources array when 'totalResults' property is not specified", () => {
assert.strictEqual(new ListResponse(Object.assign([], {length: 100})).totalResults, 100,
"Instance member 'totalResults' did not equal 'length' property of resources array when 'totalResults' property was not specified");
});

it("should prefer 'totalResults' property over 'length' property of resources array when preparing outbound messages", () => {
assert.strictEqual(new ListResponse(Object.assign([], {length: 1000, totalResults: 100})).totalResults, 100,
"Instance member 'totalResults' did not prefer 'totalResults' property over 'length' property of resources array when preparing outbound messages");
});
});
});
Loading

0 comments on commit f8a3416

Please sign in to comment.