-
-
Notifications
You must be signed in to change notification settings - Fork 67
Delete scopes #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Delete scopes #181
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ const lock = new (require("rwlock"))(); | |
| const ioOperations = require("./io-operations.js"); | ||
| const { getConfig } = require("./config"); | ||
|
|
||
| class NoMatchingScopesError extends Error {} | ||
|
|
||
| const isImportMap = () => { | ||
| const format = getConfig().manifestFormat; | ||
| if (format === "importmap") { | ||
|
|
@@ -178,6 +180,26 @@ exports.modifyService = function ( | |
| }); | ||
| }; | ||
|
|
||
| exports.NoMatchingScopesError = NoMatchingScopesError; | ||
| exports.deleteScopes = function (env, regex) { | ||
| return modifyLock(env, (json) => { | ||
| // I'm expecting this to always maintain the order of the keys | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the order of the keys matter in import map scopes?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No they don't but I wanted to respect the alphabetical order config; which applies ordering to scopes on PATCH /import-map.json and PATCH /services |
||
| // note: pure numeric keys will always sort themselves | ||
| const filteredScopes = Object.fromEntries( | ||
| Object.entries(json.scopes).filter(([scopeKey]) => !regex.test(scopeKey)) | ||
| ); | ||
|
|
||
| if ( | ||
| Object.keys(filteredScopes).length === Object.keys(json.scopes).length | ||
| ) { | ||
| throw new NoMatchingScopesError(); | ||
| } | ||
|
|
||
| json.scopes = filteredScopes; | ||
| return json; | ||
| }); | ||
| }; | ||
|
|
||
| exports.getEmptyManifest = getEmptyManifest; | ||
|
|
||
| function sortObjectAlphabeticallyByKeys(unordered) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -332,6 +332,75 @@ app.delete("/services/:serviceName", function (req, res) { | |||
| }); | ||||
| }); | ||||
|
|
||||
| app.delete("/scopes", function (req, res) { | ||||
| let body; | ||||
| if (typeof req.body === "string") { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what situation would the request body not be a string? Is this if statement for when there is an empty request body? As far as I can tell, request bodies are always parsed as a string in this project, due to the line of code below. import-map-deployer/src/web-server.js Line 38 in b6c0ca7
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes! In that case, req.body is an empty object. I didn't want to mess around with the middleware; but that oddity is making the code hard to reason about. |
||||
| try { | ||||
| body = JSON.parse(req.body); | ||||
| } catch (e) { | ||||
| return res.status(400).send("Invalid request body"); | ||||
| } | ||||
| } else { | ||||
| body = req.body; | ||||
| } | ||||
|
|
||||
| if (body.regexString === undefined) { | ||||
| return res | ||||
| .status(400) | ||||
| .send("The regex key in the request body must have a value"); | ||||
| } | ||||
| if (typeof body.regexString !== "string") { | ||||
| return res.status(400).send("The regex value must be a string"); | ||||
| } | ||||
| if (body.regexString.length === 0) { | ||||
| return res.status(400).send("The regex value must not be an empty string"); | ||||
| } | ||||
|
|
||||
| let regex; | ||||
|
|
||||
| try { | ||||
| regex = new RegExp(body.regexString); | ||||
| } catch (e) { | ||||
| return res.status(400).send("The regex value is not valid regex"); | ||||
| } | ||||
|
|
||||
| const env = getEnv(req); | ||||
| const isDryRun = req.query.dry_run === "true" || req.query.dry_run === ""; | ||||
|
|
||||
| if (isDryRun) { | ||||
| return ioOperations | ||||
| .readManifest(env) | ||||
| .then((data) => { | ||||
| let scopesToBeDeleted = Object.fromEntries( | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
| Object.entries(JSON.parse(data).scopes).filter(([key]) => | ||||
| regex.test(key) | ||||
| ) | ||||
| ); | ||||
| res.send(scopesToBeDeleted); | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This endpoint should send the same response data regardless of whether the dry run query parameter is provided, but it doesn't. This should send the full import map to match when the query parameter is missing |
||||
| }) | ||||
| .catch((ex) => { | ||||
| sendError( | ||||
| res, | ||||
| ex, | ||||
| "Unexpected error while collecting the deleted scopes for this dry run" | ||||
| ); | ||||
| }); | ||||
| } | ||||
|
|
||||
| return modify | ||||
| .deleteScopes(env, regex) | ||||
| .then((data) => { | ||||
| res.send(data); | ||||
| }) | ||||
| .catch((ex) => { | ||||
| if (ex instanceof modify.NoMatchingScopesError) { | ||||
| return res.status(404).send("No matching scopes found"); | ||||
| } else { | ||||
| sendError(res, ex, "Could not delete scope(s)"); | ||||
| } | ||||
| }); | ||||
| }); | ||||
|
|
||||
| let server; | ||||
| if (process.env.NODE_ENV !== "test") { | ||||
| server = app.listen( | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,3 +103,234 @@ describe(`/import-map.json - Scopes`, () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe(`DELETE /scopes`, () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test where the scopes are fully qualified urls |
||
| let errorSpy; | ||
| beforeEach(async () => { | ||
| errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); | ||
| errorSpy.mockClear(); | ||
| const response = await request(app) | ||
| .patch("/import-map.json") | ||
| .query({ | ||
| skip_url_check: true, | ||
| }) | ||
| .set("accept", "json") | ||
| .send({ | ||
| imports: { | ||
| a: "https://cdn.com/a/3.1.1/a.js", | ||
| b: "https://cdn.com/b/1.2.5/b.js", | ||
| shared: "https://cdn.com/shared@22/shared.js", | ||
| }, | ||
| scopes: { | ||
| "/a/1.0.0/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/a/2.0.0/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/a/3.1.1/": { | ||
| shared: "https://cdn.com/shared@19/shared.js", | ||
| }, | ||
| }, | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
|
|
||
| expect(response.body).toMatchObject({ | ||
| imports: { | ||
| a: "https://cdn.com/a/3.1.1/a.js", | ||
| b: "https://cdn.com/b/1.2.5/b.js", | ||
| shared: "https://cdn.com/shared@22/shared.js", | ||
| }, | ||
| scopes: { | ||
| "/a/1.0.0/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/a/2.0.0/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/a/3.1.1/": { | ||
| shared: "https://cdn.com/shared@19/shared.js", | ||
| }, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it(`Returns a 400 if there is no request body`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send() | ||
| .expect(400); | ||
| }); | ||
|
|
||
| it(`Returns a 400 if the request body doesn't have a regexString key`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| somethingInvalid: 123, | ||
| }) | ||
| .expect(400); | ||
| }); | ||
|
|
||
| it(`Returns a 400 if the regexString is not a string`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| regexString: 123, | ||
| }) | ||
| .expect(400); | ||
| }); | ||
|
|
||
| it(`Returns a 400 if the regexString is an empty string`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| regexString: "", | ||
| }) | ||
| .expect(400); | ||
| }); | ||
|
|
||
| it(`Returns a 400 if the regexString is invalid`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| regexString: "something[", | ||
| }) | ||
| .expect(400); | ||
| }); | ||
|
|
||
| it(`Returns a 404 if the regex doesn't match any scopes`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| regexString: "/c/[^/]+/", | ||
| }) | ||
| .expect(404); | ||
| }); | ||
|
|
||
| it(`Returns a 404 if the provided env doesn't exist`, async () => { | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .query({ env: "envDoesNotExist" }) | ||
| .send({ | ||
| regexString: "/c/[^/]+/", | ||
| }) | ||
| .expect(404); | ||
| }); | ||
|
|
||
| it(`Returns an empty object if using the dry_run flag and the regex doesn't match any scopes`, async () => { | ||
| const response = await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .query({ dry_run: true }) | ||
| .send({ | ||
| regexString: "/c/[^/]+/", | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
|
|
||
| expect(response.body).toMatchObject({}); | ||
| }); | ||
|
|
||
| it(`Returns a map of the scopes that would be deleted when using the dry_run flag`, async () => { | ||
| const response = await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .query({ dry_run: true }) | ||
| .send({ | ||
| // matches scopes that contain /a/{any string except "3.1.1"}/ | ||
| regexString: "/a/(?!3\\.1\\.1/)[^/]+/", | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
|
|
||
| expect(response.body).toMatchObject({ | ||
| "/a/1.0.0/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/a/2.0.0/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it(`Deletes the correct scopes and returns the newly updated import-map`, async () => { | ||
| const response = await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| // matches scopes that contain /a/{any string except "3.1.1"}/ | ||
| regexString: "/a/(?!3\\.1\\.1/)[^/]+/", | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
|
|
||
| expect(response.body).toMatchObject({ | ||
| imports: { | ||
| a: "https://cdn.com/a/3.1.1/a.js", | ||
| b: "https://cdn.com/b/1.2.5/b.js", | ||
| shared: "https://cdn.com/shared@22/shared.js", | ||
| }, | ||
| scopes: { | ||
| "/a/3.1.1/": { | ||
| shared: "https://cdn.com/shared@19/shared.js", | ||
| }, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it(`Deleting scopes doesn't effect the original order of the scopes`, async () => { | ||
| // Add some additional scopes | ||
| await request(app) | ||
| .patch("/import-map.json") | ||
| .query({ | ||
| skip_url_check: true, | ||
| }) | ||
| .set("accept", "json") | ||
| .send({ | ||
| scopes: { | ||
| "/b/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/c/": { | ||
| shared: "https://cdn.com/shared@11/shared.js", | ||
| }, | ||
| "/d/": { | ||
| shared: "https://cdn.com/shared@19/shared.js", | ||
| }, | ||
| }, | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
| // Delete /a/2.0.0/, inserted from the beforeEach | ||
| await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| regexString: "^/a/2.0.0/$", | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
| // Delete /c/ | ||
| const response = await request(app) | ||
| .delete("/scopes") | ||
| .set("accept", "json") | ||
| .send({ | ||
| regexString: "^/c/$", | ||
| }) | ||
| .expect(200) | ||
| .expect("Content-Type", /json/); | ||
|
|
||
| const actualScopeKeys = Object.keys(response.body.scopes); | ||
| const expectedScopeKeys = ["/a/1.0.0/", "/a/3.1.1/", "/b/", "/d/"]; | ||
|
|
||
| expect(actualScopeKeys).toEqual(expectedScopeKeys); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the regexString being sent in the request body rather than encoded in the url path? I first expected to see
DELETE /scopes/:urlEncodedScopeas the API rather than a request body. I've never used request bodies with the HTTP DELETE method before, and some sources (including MDN) suggest that DELETE requests shouldn't have a request body.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with you that it's definitely not normal; but then yeah it requires url encoding as you mentioned... Semi-annoying to type if you want to make manual calls to the API (encoding the regex). I'm good with both approaches (assuming we want to do regex).