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

Fix some bugs with encoding #901

Merged

Conversation

Martii
Copy link
Member

@Martii Martii commented Feb 21, 2016

NOTES

  • Followup with DB migration to add fork.utf to existing forks otherwise they may not show up next to the bullet (basic copy done... most manually fixed and validated... may be a few left)
  • Leaving GH import alone as it still works so far ... however a watchpoint added... GH web UI always encodeURIComponents just about everything... will need to reaffirm existing encodeURIs later
  • The term uri is inclusive to urls as previously described in Fix installName vs installNameSlug references #819 ... however in this context url means it could be partially encoded whereas uri means it absolutely is encoded... except for the slashes... normally those are encoded but we don't want them encoded... again following GH example. A url containing ä may be just that however a uri has %E4. You will notice that I renamed some of our identifiers from url to uri... this is on purpose to indicate that is what we are expecting.
  • express redirect() uses Location and Content-Location headers. This requires all urls to be at least URI encoded as per specs... e.g. all the browsers are adding in an escape for the section symbol which may be an issue with them. So hence why the naming of url vs uri.
  • The base issue of readability of the code is still there... but at least this punches out some of the bugs mentioned.
  • Compression/optimization hasn't occurred yet
  • No DB migration is necessary now... this includes fixing Re: linkage
  • The smarter encoder isn't finished yet as I may be rearranging this a bit but...

This should knock out the BLOCKING label I put on... at least from my perspective.

Applies to #819 and #262... treads on #262 (comment) among many others.

* Remove legacy namespace in all routes ... part of OpenUserJS#262 and OpenUserJS#130... officially EOL as per sizzle and confirmed by me... no redirects.
* Remove the patches I put in for this portion
* Create function for encoding similar to GH's... see helper `encoding`
* Using encodeURIComponent from OpenUserJS#795
* Fix bug of Fork History showing "slugged" instead of native/raw
* Fix category reference bug... redirect was set to the Object itself and not one of its properties
* Fix some more missing object identifiers in modelParser.js ... not all are validated yet and some may still be missing for future proofing
* Remove and Add some more watchpoints
* Flip successful "rating updated" for Groups to debug mode and related in `./libs/modelParser.js` ... these are generating too much log traffic OpenUserJS#430

**NOTES**
* [ ] Followup with DB migration to add `fork.utf` to existing forks otherwise they may not show up next to the bullet
* Leaving GH import alone as it still works so far ... however a watchpoint added... GH web UI always `encodeURIComponent` just about everything... will need to reaffirm existing `encodeURI` later
* The term uri is inclusive to urls as previously described in OpenUserJS#819 ... however in this context url means it could be partially encoded whereas uri means it absolutely is encoded... except for the slashes... normally those are encoded but we don't want them encoded... again following GH example. A url containing `ä` may be just that however a uri has `%E4`. You will notice that I renamed some of our identifiers from `url` to `uri`... this is on purpose to indicate that is what we are expecting.
* *express* `redirect()` uses [`Location`](http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.30) and [`Content-Location`](http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.14) headers. This requires all urls to be at least URI encoded as per specs... e.g. all the browsers are adding in an `escape` for the section symbol *which may be an issue with them*. So hence why the naming of `url` vs `uri`.
* The base issue of readability of the code is still there... but at least this punches out some of the bugs mentioned.
* Compression/optimization hasn't occurred yet
* No DB migration is necessary now... this includes fixing `Re:` linkage
* The smarter encoder isn't finished yet as I may be rearranging this a bit but...

**This should knock out the BLOCKING label I put on**... at least from my perspective.

Applies to OpenUserJS#819 and OpenUserJS#262... treads on OpenUserJS#262 (comment) among many others.
@Martii Martii added bug You've guessed it... this means a bug is reported. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. labels Feb 21, 2016
Martii added a commit that referenced this pull request Feb 21, 2016
…taticUris

Fix some bugs with encoding

Auto-merge with several hours of testing on dev with some local pro
@Martii Martii merged commit 0d06785 into OpenUserJS:master Feb 21, 2016
@Martii Martii deleted the Issue-819adaptiveEncodingUrlsAndStaticUris branch February 21, 2016 08:01
Martii pushed a commit to Martii/UserScripts that referenced this pull request Feb 22, 2016
* Remove stray comment
* Collecting some webhook data specific to OpenUserJS/OpenUserJS.org#901
'Webhook filename:',
' ' + aFilename
].join('\n')); // TODO: After some data collected, reaffirm and remove this
repo[aFilename] = '/' + encodeURI(aFilename); // NOTE: Watchpoint
Copy link
Member Author

Choose a reason for hiding this comment

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

Some data here:

aFilename := src/RFC 2606§3/RFC 2606§3 - ä %, Unit Test/RFC 2606§3 - ä %, Unit Test.user.js on trigger.

Refs:

  1. 0d06785/libs/repoManager.js#L119-L120
  2. https://raw.githubusercontent.com/Martii/UserScripts/master/src/RFC%202606%C2%A73/RFC%202606%C2%A73%20-%20%C3%A4%20%25%2C%20Unit%20Test/RFC%202606%C2%A73%20-%20%C3%A4%20%25%2C%20Unit%20Test.user.js (Raw URL copied from address bar)
  3. encodeURI('RFC 2606§3 - ä %, Unit Test.user.js') := RFC%202606%C2%A73%20-%20%C3%A4%20%25,%20Unit%20Test.user.js
  4. encodeURIComponent('RFC 2606§3 - ä %, Unit Test.user.js') := RFC%202606%C2%A73%20-%20%C3%A4%20%25%2C%20Unit%20Test.user.js
  5. /Martii/UserScripts/raw/master/src/RFC%202606%C2%A73/RFC%202606%C2%A73%20-%20%C3%A4%20%25%2C%20Unit%20Test/RFC%202606%C2%A73%20-%20%C3%A4%20%25%2C%20Unit%20Test.user.js (the href attribute from here on the Raw button)

@Zren Cc: @sizzlemctwizzle
Would either of you say there is an issue here? (EDITED: look for the comma... e.g. %2C and , respectively ... in list item 2, 3, 4 and 5 )

Everything appears alright on pro with the JSON... but I've also picked simple values in the Userscript and something still doesn't feel right.

Martii pushed a commit to Martii/OpenUserJS.org that referenced this pull request Feb 24, 2016
* Post first 100 chars of body for relase *express-minify* on error... id what's occasionally happening. Hopefully this is enough.
* Remove logging from OpenUserJS#901... got some data and posted Unit Test data at https://github.com/OpenUserJs/OpenUserJS.org/pull/901/files#r53597717
* Expand single removal message to be more specific

Applies to OpenUserJS#430
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge.
Development

Successfully merging this pull request may close these issues.

1 participant