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

Migrate the scripts in S3 to use script._id instead of the path value #486

Open
Martii opened this issue Dec 6, 2014 · 10 comments
Open
Labels
bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. DB Pertains inclusively to the Database operations. migration Use this to indicate that it may apply to an existing or announced migration. needs discussion Blah, blah, blah, wahh, wahh, wahh, etc.
Milestone

Comments

@Martii
Copy link
Member

Martii commented Dec 6, 2014

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:

@Martii Martii added DB Pertains inclusively to the Database operations. bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. needs discussion Blah, blah, blah, wahh, wahh, wahh, etc. migration Use this to indicate that it may apply to an existing or announced migration. team biz This is similar to a meta discussion. labels Dec 6, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Dec 6, 2014
* 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
@Martii
Copy link
Member Author

Martii commented Dec 6, 2014

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 installName and migrate) from what we store in the Raw JSON Data or we can just lowercase migrate everything username wise... It is probably not recommended that we should alter the scriptname value though in casing with the Key.

Updated the above for some test points and other linkage.

@Zren
Copy link
Contributor

Zren commented Dec 6, 2014

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 Script._id either as that means only 1 source code per Script. But since we're not versioning it atm, we could just use that.

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 callback(err, ...) pattern so we can chain error up to the route handlers.

@sizzlemctwizzle
Copy link
Member

I agree with @Zren. We should use Script._id instead since we have to do a lookup anyway.

@Martii
Copy link
Member Author

Martii commented Dec 7, 2014

Create an id for each source code and store it in the Script model.

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.

@Martii
Copy link
Member Author

Martii commented Dec 9, 2014

Just a FYI here's a short snippet from recent production log with the isPro console message I added in to check for this on pro e.g. the version of aws-sdk we are currently using hides/mishandles the error and possibly returns an empty payload as one of the maintainers stated... jri's script does show source and does install.

[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.

@Martii
Copy link
Member Author

Martii commented Apr 27, 2015

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
Any objections to trying it on production? I'll give this a day or so for any responses.

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Apr 27, 2015
Appears to work in dev and local pro... not sure about production

See also:
* OpenUserJS#486 (comment)
@sizzlemctwizzle
Copy link
Member

I have no problem with a temporary fix, but we definitely need to migrate the scripts in S3 to use script._id instead of the path value somewhere down the line. It's a simple migration but I'm always afraid I'm going to break something when I mess with S3. I spent hours trying to get everything right when I removed namespaces.

@Martii
Copy link
Member Author

Martii commented Apr 27, 2015

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:

@Martii Martii changed the title Normalize username casing for DB storage Migrate the scripts in S3 to use script._id instead of the path value Apr 27, 2015
@Martii Martii removed the team biz This is similar to a meta discussion. label Apr 27, 2015
@Martii
Copy link
Member Author

Martii commented Apr 27, 2015

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.

@Martii Martii added sooner Sooner would be appreciated. and removed needs discussion Blah, blah, blah, wahh, wahh, wahh, etc. labels Jun 29, 2015
@Martii Martii added this to the #486 milestone Jul 2, 2015
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 7, 2015
* **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
@Martii Martii mentioned this issue Jul 7, 2015
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 7, 2015
* Still used to a slightly different coding style myself...apologies

Applies to OpenUserJS#658, OpenUserJS#656, OpenUserJS#486, and OpenUserJS#180
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Aug 18, 2015
* 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.
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Dec 16, 2015
* 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.
@Martii Martii added needs discussion Blah, blah, blah, wahh, wahh, wahh, etc. and removed sooner Sooner would be appreciated. labels Apr 19, 2017
@Martii
Copy link
Member Author

Martii commented Apr 19, 2017

There's a major consequence to doing this... if script._id is ever used as the key in S3, then if the DB ever needs to be "moved" somewhere all of the _ids could be different depending on how it was restored. If it's a DB backup and then a restore then it might be okay if it preserves the same _ids; but if it's a copy/delete op all of the _ids will be different and all source will be orphaned.

NOTE: This is happening less and less as time progresses and fixes roll through.

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Apr 24, 2017
* 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
Martii added a commit that referenced this issue Apr 24, 2017
* Since #1086 haven't seen any of these yet for #486
* Use an environment variable to toggle monitoring this for now


Applies to #430

Auto-merge
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue May 1, 2017
* 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
Martii added a commit that referenced this issue May 1, 2017
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. DB Pertains inclusively to the Database operations. migration Use this to indicate that it may apply to an existing or announced migration. needs discussion Blah, blah, blah, wahh, wahh, wahh, etc.
Development

No branches or pull requests

3 participants