Skip to content

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

Closed
jacargentina wants to merge 1 commit intoapache:masterfrom
jacargentina:detect-malformed-id
Closed

Return better error when doc._id is not decodable with decodeURIComponent#2545
jacargentina wants to merge 1 commit intoapache:masterfrom
jacargentina:detect-malformed-id

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
Contributor

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
Contributor

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
Contributor

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

3 participants