Conversation
| @@ -0,0 +1,328 @@ | |||
| /* | |||
There was a problem hiding this comment.
We should never include "edgekv.js" in any of our public examples. Instead, the documentation should instruct the user to download the latest from https://github.com/akamai/edgeworkers-examples/tree/master/edgekv/lib
|
|
||
| export async function responseProvider (request) { | ||
| // Set Up EdgeKV | ||
| const edgeKv = new EdgeKV({namespace: "url-shortener", group: "links"}); |
There was a problem hiding this comment.
Recommend moving this as a global.
| @@ -0,0 +1,7 @@ | |||
| var edgekv_access_tokens = { | |||
| "namespace-default": { | |||
There was a problem hiding this comment.
Recommend either not providing a generic edgekv_tokens.js or instead renaming this to "namespace-url-shortener" to be consistent with the namespace used in main.js.
|
|
||
| // If the request path contains /r/ then we are serving a redirect | ||
| if (path.split('/')[1] === 'r') { | ||
| let key = path.split('/')[2]; |
There was a problem hiding this comment.
I suggest you sanitize the key here since it is passed in by a user without any further checks, so could present an opportunity for exploits.
| // If the request path contains /m/ then we are managing redirects | ||
| if (path.split('/')[1] === 'm') { | ||
| var params = new URLSearchParams(request.query); | ||
| var name = params.get('name'); |
There was a problem hiding this comment.
suggest sanitizing the name here as well
|
|
||
| try { | ||
| // Check to see if the redirect exists before we attempt to delete it | ||
| checkID = await edgeKv.getText({ item: name, default_value: undefined }); |
There was a problem hiding this comment.
This is an anti pattern in EdgeKV. Due to the eventual consistency properties of EdgeKV, there is no guarantee that this will return a value even if the item was created by the same edge server. As such, I suggest collapsing the create and update cases and having the URL checking be tracked by the user outside of EdgeKV or not checked at all. In the latter case you will always create a new short URL or update the existing one if the name exists, but this could happen anyway (and very likley in fact) even if you call getText. So calling getText is both misleading the users and also adding unnecessary latency.
| body = '<b>' + status + '<b><br>'; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
There is absolutely no need to check existence of the item before deleting it, calling delete will delete if it exists or be a no-op essentially. In fact it is even worse to check since in cases when the item was just recently created, the check may not retrun anything (due to eventual consistency property of EdgeKV) and then the delete would not be called. Whereas calling delete without any checks is guaranteed to delete an item if it exists regardless.
2 participants
No description provided.