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

unique check: pass in actual value, not string version #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

makii42
Copy link

@makii42 makii42 commented Apr 3, 2017

Hi,

I was trying to enforce uniqueness on a int32 field in a model struct using models/context, and it just did not work, without a proper error message. While looking at the mechanism enforcing uniqueness I discovered the example query that is executed in collection.go:Unique This function gets passed the String()ed version of the reflect.Value, rather than the underlying value of the field. That might work for strings, but apparently does not for int fields. I found two instances where this happens:

Instead of passing the result of Value.String() I propose to pass Value.Interface() to make this work for non-string fields as well.

@diegogub
Copy link
Owner

diegogub commented Apr 4, 2017

hi @makii42, thanks. Let me check it.

@makii42
Copy link
Author

makii42 commented Apr 4, 2017

Hi @diegogub ,

I looked into the test failure yesterday for a while, and it seems there is no arangodb running as part of the test. I prepped a wercker.yml locally that's working, and added some code to config_test.go that uses a db at localhost and flips over to one provided in a service container if it's running in wercker. I'll add it to the PR in a bit.

Unfortunately, one test still fails in simple.test.go when using Cursor.FetchOne(). This seems unrelated to my change though.

Best,
-Ralf

@makii42
Copy link
Author

makii42 commented Apr 4, 2017

Okay, that's weird - I just set up a wercker build on my fork, and it gets as far as I get when I use the wercker client locally... wercker status

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.

2 participants