Skip to content
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

[download-microservice] Refactor: Clarify Execution, make bin finding clearer #132

Merged
merged 9 commits into from
Feb 4, 2024
88 changes: 88 additions & 0 deletions microservices/download/bins.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// A module that defines the way we locate a specific binary file amid all possible
// Pulsar Rolling Release binaries.
// In order to do this the following keys are available:
// - startsWith: Checks if the version string startsWith the value
// - endsWith: Checks if the version string endsWith the value
// - endsWithNot: Checks if the version string does not end with the value

module.exports = {
windows: {
windows_setup: {
startsWith: "Pulsar.Setup",
endsWith: ".exe"
},
windows_portable: {
endsWith: "-win.zip"
},
windows_blockmap: {
startsWith: "Pulsar.Setup",
endsWith: ".exe.blockmap"
}
},

silicon_mac: {
mac_zip: {
endsWith: "-arm64-mac.zip"
},
mac_zip_blockmap: {
endsWith: "-arm64-mac.zip.blockmap"
},
mac_dmg: {
endsWith: "-arm64.dmg"
},
mac_dmg_blockmap: {
endsWith: "-arm64.dmg.blockmap"
}
},

intel_mac: {
mac_zip: {
endsWith: "-mac.zip",
endsWithNot: "-arm64-mac.zip"
},
mac_zip_blockmap: {
endsWith: "-mac.zip.blockmap",
endsWithNot: "-arm64-mac.zip.blockmap"
},
mac_dmg: {
endsWith: ".dmg",
endsWithNot: "-arm64.dmg"
},
mac_dmg_blockmap: {
endsWith: ".dmg.blockmap",
endsWithNot: "-arm64.dmg.blockmap"
}
},

arm_linux: {
linux_appimage: {
endsWith: "-arm64.AppImage"
},
linux_tar: {
endsWith: "-arm64.tar.gz"
},
linux_rpm: {
endsWith: ".aarch64.rpm"
},
linux_deb: {
endsWith: "_arm64.deb"
}
},

linux: {
linux_appimage: {
endsWith: ".AppImage",
endsWithNot: "-arm64.AppImage"
},
linux_tar: {
endsWith: ".tar.gz",
endsWithNot: "-arm64.tar.gz"
},
linux_rpm: {
endsWith: ".x86_64.rpm"
},
linux_deb: {
endsWith: "_amd64.deb"
}
}
};
57 changes: 45 additions & 12 deletions microservices/download/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,59 @@ const utils = require("./utils.js");
const port = parseInt(process.env.PORT) || 8080;

const server = http.createServer(async (req, res) => {
// Since req.url is our URL pluse query params, lets split them first.
const path = req.url.split("?");
if (typeof req?.url !== "string"){
// Not sure if this error condition is even possible, but handling it anyway
await utils.displayError(req, res, {
code: 500,
msg: "Internal Server Error: Misformed URL"
});
console.log("Download Returned 500 due to the requested URL not being received as a string internally");
return;
}

// Since req.url is our URL plus query params, lets split them first.
const url = req.url.split("?");
const path = url[0];
const queryParams = url[1];
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

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

If you want to keep these const definitions and otherwise meld my changes of index.js into this PR it should apply pretty cleanly in terms of like diff conflicts and such.

Either this way or my branch are more readable than before, IMO. Your version here might be even slightly nicer, it's pretty subjective tho. path and queryParams seems very readable, IMO, as you have it on this branch already. 👍

Copy link
Member

@DeeDeeG DeeDeeG Feb 2, 2024

Choose a reason for hiding this comment

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

Maybe too much thoughts on this, tl;dr I think your var names are better.

I ended up with this on my branch (snippet), which I wasn't in love with but couldn't think of how to get exactly the names I wanted and the readability I wanted:

  let [urlPath, urlParams] = req.url.split("?");

The destructuring assignment on my branch, while concise, is IMO more confusing and less readable.

And not much point in reminding where these consts came from (urlPath and urlParams) by keeping "url" in their names, when path and queryParams are so descriptive, without fluff and readable.

So that's my thoughts why your variable names/assignment approach works better, IMO.


EDIT to add: I can draft a patch and offer it for your consideration to merge my favorite bits of the two versions of index.js if you haven't already done it?

EDIT 2: We discussed it on Discord. This became #133 targeting for inclusion in this PR's branch directly.


// Set our Request Route
if (path[0] === "/" && req.method === "GET") {
if (path === "/" && req.method === "GET") {

if (typeof queryParams !== "string"){
await utils.displayError(req, res, {
code: 400,
msg: "Missing Query Parameters"
});
console.log("Download Returned 400 due to the requested URL having no query parameters");
return;
}

if (queryParams.length > 100){
// Longest valid combo is 36 characters ("os=silicon_mac&type=mac_zip_blockmap"),
// But leaving extra room to update the parameters in the future.
await utils.displayError(req, res, {
code: 414,
msg: "Requested Parameters are Too Long"
});
console.log("Download Returned 414 due to the provided parameters being too long");
return;
}

let params = {
os: utils.query_os(path[1]),
type: utils.query_type(path[1])
let validatedParams = {
os: utils.query_os(queryParams),
type: utils.query_type(queryParams)
};

if (!params.os || !params.type) {
if (!validatedParams.os || !validatedParams.type) {
await utils.displayError(req, res, {
code: 503,
msg: "Missing Required Download Parameters"
code: 400,
msg: "Required Download Parameters Missing or Invalid"
});
console.log("Download Returned 503 due to missing os or type");
console.log("Download Returned 400 due to missing or invalid os or type");
return;
}

let redirLink = await utils.findLink(params.os, params.type);
let redirLink = await utils.findLink(validatedParams.os, validatedParams.type);

if (!redirLink.ok) {
await utils.displayError(req, res, redirLink);
Expand All @@ -34,7 +67,7 @@ const server = http.createServer(async (req, res) => {
Location: redirLink.content
}).end();

console.log(`Download Returned: OS: ${params.os} - TYPE: ${params.type} - URL: ${redirLink.content}`);
console.log(`Download Returned: OS: ${validatedParams.os} - TYPE: ${validatedParams.type} - URL: ${redirLink.content}`);


} else {
Expand Down
Loading
Loading