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

id should be url encoded when used in the delete url #43

Open
LaurentVB opened this issue Apr 4, 2016 · 4 comments
Open

id should be url encoded when used in the delete url #43

LaurentVB opened this issue Apr 4, 2016 · 4 comments
Labels

Comments

@LaurentVB
Copy link

Hello.
We use the email address as id for users that want to subscribe to the newsletter but have no user account.
When the user subsequently creates a real account, we delete the existing record based on email address and add a new one with an integer id.

# Remove possible newsletter signup from customer
Customerio::Client.new(id,key).delete(user.email)

We recently have a user that has an accented character in the "local" part of the email address (ex: abcdéf@domain.com). The delete call fails with the following error:

An URI::InvalidURIError occurred in signups#create:
 bad URI(is not URI?): /api/v1/customers/abcdéf@domain.com
 app/models/user_observer.rb:14:in `after_create'

I'm not sure that this is a valid email address (I probably is according to the RFC), but I think you should probably make the lib more robust and url encode the passed id.

@alisdair
Copy link
Contributor

alisdair commented Apr 6, 2016

I'm not sure that we can do anything about this, unfortunately, as it would be backwards-incompatible. Anyone using data which requires URI encoding would already be using URI encoding themselves, and double-encoding it would cause a failure.

As a workaround, you should be able to run URI.encode(id) on the id of your users.

If you can think of a backwards-compatible way to do this, please let me know.

@alisdair alisdair added the bug label Apr 6, 2016
@LaurentVB
Copy link
Author

Hi.
Indeed, backwards-compatibility would be an issue.
I see three options:

  • Detect if the string has already been encoded. If not, encode it (http://stackoverflow.com/a/2295286/424352 should work in 99,9% of the cases, shouldn't it?)
  • add some config option in the CustomerIO::Client class that would force to always encode what's used in the url (some kind of strict mode, which would be off by default for backwards-compatibility, until next major release)
  • if you consider it's the developer's responsibility and not the lib's to ensure the urls are valid, then you can simply document that every passed parameter that's used in URL must be urlencoded

In any case, that will require some documentation. Also maybe see what other Customer.io libs in other languages are doing, probably better to stay consistent.

Cheers.

@alisdair
Copy link
Contributor

I've surveyed what the other Customer.io libraries are doing, and they all expect the id parameter to be URL encoded by the developer. For consistency that's the approach we're going to take here as well.

I'll leave this bug open until I update the documentation to reflect this. Thank you for the report and follow-up!

@LaurentVB
Copy link
Author

Makes sense then.
Thanks for not having closed the issue right away as a user error ;-)

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

No branches or pull requests

2 participants