-
Notifications
You must be signed in to change notification settings - Fork 309
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
Migrate the scripts in S3 to use script._id
instead of the path value
#486
Comments
* Directly related to OpenUserJS#37. Stops net timeout when picking some other script that isn't present in fakeS3. Eventually when *aws-sdk* is updated this will be needed so it doesn't halt dev. Possibly applicable routine to OpenUserJS#486 for inline live migration
With #487 we have another option of inline live migration... that patch currently fixes the net timeout in dev when selecting some other script source with fakeS3 and it usually just spins the browser spinner for an incredibly long time since our local dev DB doesn't usually have that source. We can either correct the casing to what the user entered, if applicable, (e.g. a quick user search in the DB and if it is a hit compare it to the username portion generated from Updated the above for some test points and other linkage. |
Normally you wouldn't base the filename/keys based on the username/scriptname. Create an id for each source code and store it in the Script model. If we wanted to future proof ourselves, we wouldn't use the We have to fetch the Script model first before we doing any AWS stuff anyways. Edit: Also noticed this when I tried to refactor the scriptEditSourceCodePage that never got finished. Most of the callbacks need to follow the |
I agree with @Zren. We should use |
Yah I'm not entirely sure how lower casing a Unicode character would affect things so a unique id would probably be best. EDIT: Searching will need to be retested for this as well. |
Just a FYI here's a short snippet from recent production log with the [12/08 23:36:21 EST][err] S3 Key Not Found jri/Geocaching_Map_Enhancements.user.js
[12/08 23:40:33 EST][out] Group(imgur) Rating Updated
[12/08 23:45:24 EST][out] Group(deviantART) Rating Updated
[12/08 23:48:10 EST][err] S3 Key Not Found mienaikage/OkChoices.user.js
[12/08 23:48:53 EST][out] Group(Subtitles) Rating Updated
[12/08 23:49:42 EST][out] Group(Steam) Rating Updated
[12/08 23:49:42 EST][out] Group(dota2lounge) Rating Updated
[12/08 23:49:47 EST][out] Group(download) Rating Updated
[12/08 23:51:02 EST][out] GitHub client authenticated
[12/08 23:51:10 EST][out] Group(bookmarks) Rating Updated Hmmm their clock appears to be a bit skewed too. |
Hmmm... retested this randomly with aws-sdk@2.1.25 and doesn't appear to crash dev nor local pro anymore... not sure on actual production. All prior test points appear to be okay (without crashing local pro). :\ @sizzlemctwizzle |
Appears to work in dev and local pro... not sure about production See also: * OpenUserJS#486 (comment)
I have no problem with a temporary fix, but we definitely need to migrate the scripts in S3 to use |
Of course the issue needs to stay open... just thought I could get the dep updated a little bit. I am on a slightly newer node.js though which is why it would need actual production testing... just got done with local pro and seems okay not to crash here. See also:
Possibly related to:
|
script._id
instead of the path value
Appears to be working on production with the dep update... will let this run for a while and recheck the production logs periodically. Still getting my error trap message but server isn't restarting... tried a few variations on casing of the username and those return 404 which was unexpected but doable I think: [04/27 02:12:11 MDT][out] Group(dota2lounge) Rating Updated
[04/27 02:12:11 MDT][out] Group(Steam) Rating Updated
[04/27 02:22:28 MDT][out] Group(Google) Rating Updated
[04/27 02:22:41 MDT][err] S3 Key Not Found trespassersw/google_cache_comeback.user.js
[04/27 02:22:59 MDT][err] S3 Key Not Found dexmaster/Checkout_Free_Books_(Amazon).user.js
[04/27 02:24:18 MDT][out] GitHub client authenticated
[04/27 02:26:18 MDT][out] Group(imgur) Rating Updated
[04/27 02:30:17 MDT][out] Group(powdertoy) Rating Updated
[04/27 02:30:22 MDT][out] Group(Shortcuts) Rating Updated
[04/27 02:31:47 MDT][err] S3 Key Not Found trespassersw/google_cache_comeback.user.js See also:
Will rollback to v0.1.7-58 snapshot if current usable functionality is hindered or continual server restarts with posted logs. |
* **NOTE** This is not the permanent solution but a compromise on existing S3 DB structure and this hot fix isn't as effecient as OpenUserJS#486 solution *(hence the term hot fix vs solution)*. * Change `caseInsensitive` regular expression maker into `caseSensitive` with exception on username to accomodate OpenUserJS#180 without the issue in OpenUserJS#656 * This removes the future security issue with unintended hidden scripts... manual DB examination will need to happen for anyone who has done this before but can be done in time with @sizzlemctwizzle and/or @Martii... future scripts should properly be created at least. * Noted code inconsistency with `installName` versus "more than `installName`"... this should be corrected at some point. * Leaving `caseInsensitive` in as a potential library function in `scriptStorage`... which could probably be in libs but that's where it is currently. Closes OpenUserJS#656
* Still used to a slightly different coding style myself...apologies Applies to OpenUserJS#658, OpenUserJS#656, OpenUserJS#486, and OpenUserJS#180
* Misplaced `var` declarations ... closer to current ES5.x implementation for some others just misplaced * Reordered on first use for all vars... not as critical but good for debugging * Changed `s3` early initialization... shouldn't need to be **and if it did** it should always be `NOTE:`d with a comment * Some newlines for white space usage... easier reading/debugging * Any `require` statements all at top... `require` functions become cached and always stay loaded after first use... so really shouldn't be attempting to optimize these in our code... only perceived benefit is startup pm time but is loaded elsewhere so no real benefit. * Some notation of `WARNING:` statement on incomplete error handling * White-space trim that my editor didn't catch on a previous commit even though it is on all the time everywhere. :\ * `WARNING:` note added for resaving script model object for no apparent reason... again should be notated. * Max line length of 100 conformance Applies with OpenUserJS#285, OpenUserJS#486, OpenUserJS#390, and OpenUserJS#19 ... perhaps some more... very minimal change in project behavior here which is why it is isolated.
* Set some DB options to their assumed defaults from http://mongodb.github.io/node-mongodb-native/2.0/api/Server.html ... relates to https://github.com/christkv/mongodb-core/issues/66#issuecomment-165052045 * Set some replset defaults to their assumed defaults from http://mongodb.github.io/node-mongodb-native/2.0/api/ReplSet.html ... relates to Automattic/mongoose#3588 (comment) * Move/change a stderr message to debug mode of *node*... clear up the logs a bit since script source may not be found just yet. This only happens on pro so far and probably deals with streams not ready yet. Applies to OpenUserJS#430 * Move S3 source not found stderr to debug mode of *node*... known issue of OpenUserJS#486. Applies to OpenUserJS#430 **NOTES** * This will be a test whereas unless it's a huge bug I can't take pro down for at least 3 days, give or take to see if this helps or hinders from OpenUserJS#845 backout.
There's a major consequence to doing this... if NOTE: This is happening less and less as time progresses and fixes roll through. |
Applies to #486 / #180 / #130 / #130 (comment) / #819 Auto-merge
* Since OpenUserJS#1086 haven't seen any of these yet for OpenUserJS#486 * Use an environment variable to toggle monitoring this for now Applies to OpenUserJS#430
* On rare occasions S3 connection goes down so improve error logging... UI will currently show 404 on script edit submit... ideally this should be 502 but will work as former. UI new scripts will bail out and reload empty editor. * MongoDB, once inside `.save` it will **not** abort unless it's a `.pre` option with the schema which we don't currently utilize due to code structure so do it last. * Improve logging for MongoDB errors... if MongoDB connection goes down script source will be saved but UI will be out of sync instead of completely absent on a request... these are even rarer than S3. * Denoted `fallthrough` comment on an error trap if User elevation didn't happen but log for Admin+ intervention. NOTES: `unmarkModified` is pretty much useless in this case scenario but was tested Applies to OpenUserJS#430 / OpenUserJS#72 Related to OpenUserJS#486
* On rare occasions S3 connection goes down so improve error logging... UI will currently show 404 on script edit submit... ideally this should be 502 but will work as former. UI new scripts will bail out and reload empty editor. * MongoDB, once inside `.save` it will **not** abort unless it's a `.pre` option with the schema which we don't currently utilize due to code structure so do it last. * Improve logging for MongoDB errors... if MongoDB connection goes down script source will be saved but UI will be out of sync instead of completely absent on a request... these are even rarer than S3. * Denoted `fallthrough` comment on an error trap if User elevation didn't happen but log for Admin+ intervention. NOTES: `unmarkModified` is pretty much useless in this case scenario but was tested Applies to #430 / #72 Related to #486 Auto-merge
Currently we are unable to upgrade the aws-sdk dependency because "something" changed that makes
getObject
to fail.This has been explained in detail at aws/aws-sdk-js#430 . With all the test deploys done trying to figure out what the steps to reproduce are it appears that normalizing the casing when we save to the DB is an option.
Looking for more options related to advancing the goal of updating the dep... e.g. perhaps we could validate the current
aReq.params.username
against the real username (name) that is typically in the Raw JSON Data for an account. e.g. a lookup to correct the casing around here... but I suspect this is going to need to be a DB migration with a BLOCKING label until it is finished.See also:
Test points:
jri
is currently in the DB notJRI
The text was updated successfully, but these errors were encountered: