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

Return better error when doc._id is not decodable with decodeURIComponent #2545

Closed
wants to merge 1 commit into from
Closed

Return better error when doc._id is not decodable with decodeURIComponent #2545

wants to merge 1 commit into from

Conversation

jacargentina
Copy link
Contributor

I've had a case where a doc._id was badly encoded (contained a % sign).

When doing db.sync(), the error was very generic; looking at the originating stack, the culprit was decodeURIComponent

This commit fixes that, providing a new errors.MALFORMED_ID to handle that.

@calvinmetcalf
Copy link
Member

We don't ever just return an error, if you need to change the message you need to still rethrow it, it will be re-caught and the error will be returned currently all this does is return the error instead of a doc.

Also this would be covered by invalid id so we don't need to make a new code, lastly we are trying towards always using native errors when possible so we probably want something more like

try {
    doc._id = decodeURIComponent(doc._id);
  }
  catch (e) {
    var err = new TypeError('unable to decode id');
    err.status = 400;
    throw err;
  }

@jacargentina
Copy link
Contributor Author

@calvinmetcalf I've tried to adapt my idea to "pouchdb structured code and programming style", by looking at how invalidIdError did the things.

However, i had the option to include the try/catch on the very invalidIdError func, but i didnt know if it would have another consequences.

I think @nolanlawson should look at my patch first.

@calvinmetcalf
Copy link
Member

so this is one of those parts of the code where you probably don't want to use to extrapolate style for the rest of the code as this is some dated code from when we didn't use JavaScript errors period, but what we try to do is pass async errors around but throw sync errors around.

But that is secondary to us trying to avoid reusing custom errors like that because they won't have stack traces, like the one you used to debug this one here.

@nolanlawson
Copy link
Member

I agree with Calvin about using a regular TypeError. About returning vs. throwing, I think you're getting tripped up by the fact that there's a coding error a few lines above where we return an error instead of throwing it.

@calvinmetcalf
Copy link
Member

@jacargentina it is confusing so starting a patch to clear up the style

calvinmetcalf added a commit to calvinmetcalf/express-pouchdb that referenced this pull request Aug 8, 2014
calvinmetcalf added a commit to calvinmetcalf/express-pouchdb that referenced this pull request Aug 8, 2014
@nolanlawson
Copy link
Member

@calvinmetcalf fixed that throwing error (b9d8644), so we should be able to throw instead of return now.

@nolanlawson
Copy link
Member

@jacargentina Hah, good catch. None of us noticed that it made no sense to even decode the doc URI. :) #2652 fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants