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

Revisions should not url_encode or at least url_encode(diff("content")) #522

Open
ccoenen opened this issue Jul 29, 2017 · 1 comment
Open
Labels
database/sequelize Somehow this is related to the database or ORM discussion Exchange of some opinions needed

Comments

@ccoenen
Copy link
Contributor

ccoenen commented Jul 29, 2017

In #521 it looks like the Revisions table does something like this:

diff(url_encode("content"))

This makes little sense, as the diffing will always yield all of the document. And those are stored in lastContent and content anyway.

This should be reversed to be

url_encode(diff("content"))

I'm not even entirely sure why the url-encode is done in the first place, but it appears to be there.

@jackycute
Copy link
Member

jackycute commented Jul 29, 2017

Hi @ccoenen
It's because the diff might contains invalid unicode pair which can cause serious problem.

Related to these lines of code:
https://github.com/hackmdio/diff-match-patch/blob/master/index.js#L1327
https://github.com/hackmdio/diff-match-patch/blob/master/index.js#L2187

@SISheogorath SISheogorath added discussion Exchange of some opinions needed database/sequelize Somehow this is related to the database or ORM labels Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database/sequelize Somehow this is related to the database or ORM discussion Exchange of some opinions needed
Projects
None yet
Development

No branches or pull requests

3 participants