iconara / cql-rb

Cassandra CQL 3 binary protocol driver for Ruby
106 stars 31 forks source link

Raise specific errors with invalid UUID cases #119

Closed russCloak closed 10 years ago

russCloak commented 10 years ago

It would be more helpful for developers integrating functionality at a higher level if errors raised for invalid UUIDs are namespaced. This adds a new Cql::Uuid::InvalidUuidError that improves two cases:

My specific use case is that I am creating an API that is on top of a cassandra datastore, so I am making use of the Cequel gem (uses this gem). I want to rescue specific errors that might happen so I can render 404, or 400 based on what happened. This error (ArgumentError) was occurring when someone would search for a record that uses a UUID as the key, but they wouldn't supply a proper UUID. Instead of rescuing ArgumentError, which is very high-level (and happened to swallow a lot more errors than it should have), it is much more useful to me if there is some kind of specific error to catch.

iconara commented 10 years ago

Thank you, it's a good suggestion. However, to ensure API compatibility it's not possible to make this change except in a major release (e.g. 3.0). The problem is that other people have written code against the current API, and their code expects ArgumentError. Changing to another exception type would break their code. You could change the exception to be a subclass of ArgumentError, which would be API compatible, but would be unfortunate because all other Cql-namespaced exceptions are subclasses of CqlError.

I think the least invasive thing to do is to make the exception a subclass of ArgumentError, and then to change the parent class to CqlError in the next major release.

russCloak commented 10 years ago

Hey @iconara, thanks for taking a look at my pull request.

I've added a commit to base InvalidUuidError on ArgumentError via a CqlArgumentError class. I figured this would be better for when a major upgrade happens - so we can replace CqlArgumentError with CqlError, and need to change as little as possible. Also, it keeps the specific errors classes for the module specific.

Please let me know if there is anything else I need to do.

iconara commented 10 years ago

Great that you added a test that showed that the error is a subclass of ArgumentError.

I'm on vacation for another week, and I might not merge this until I'm back, just so you know.

iconara commented 10 years ago

I believe this has been merged into the new driver.