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

Too many unsafe UUID.fromString(id.value) statements. #82

Open
stewSquared opened this issue Oct 25, 2017 · 4 comments
Open

Too many unsafe UUID.fromString(id.value) statements. #82

stewSquared opened this issue Oct 25, 2017 · 4 comments

Comments

@stewSquared
Copy link
Contributor

stewSquared commented Oct 25, 2017

In the short term, some repos (reimbursements, patients) have a method that help with this:

// unsafe because of types and the possibility of fromString failure
def unsafeUUIDfromId[T](id: Id[T]) = java.util.UUID.fromString(id.value)

Though said method should be used with an explicit type.

Perhaps an even better solution could arise from creating an easy way to instantiate a MappedColumnType for specific Id types.

Edit: didn't mean to name method fromString

@jodersky
Copy link
Member

I don't quite understand this issue? Is it related to slick?

@stewSquared
Copy link
Contributor Author

stewSquared commented Oct 26, 2017

Yep. We might have a model with a typed Id that maps to a uuid field in slick. When we do something like:

Cancers.filter(_.patientId === UUID.fromString(id.value))

There's no guarantee that id is an Id[Patient]. This is fine once or twice if we're careful, but this is happening many times in per file, or even multiple times in a query.

If you instead write:

Cancers.filter(_.patientId == UUIDfromId[Patient](id))

the code is more semantic, and the id passed in is type-checked. Of course, you could technically still write incorrect code like (_.patientId === UUIDfromId[Cancer](cancer.id)). It type-checks, but it's a lot more obviously incorrect from a brief glance. We can get a bit more creative and probably eliminate that case too, but at the very least, we limit instances of UUID.fromString.

@vuspenskiy
Copy link
Contributor

@jodersky Stewart wants UuidId as in the PDS UI, if I understand correctly.

@stewSquared
Copy link
Contributor Author

@vuspenskiy haha, thanks for noticing, though that's less of an issue in this codebase 😛
I want to emphasize the Id[Patient] type safety here.

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

No branches or pull requests

3 participants