asdine / storm

Simple and powerful toolkit for BoltDB
MIT License
2.07k stars 139 forks source link

"unique" tag gets ignored, if I add a json tag #200

Closed riesinger closed 6 years ago

riesinger commented 6 years ago

Hey guys, I have a User struct, that I want to store in a storm DB and also send as a response to API queries. I use the JSON Marshaler on storm (per default) and also send the user struct to my clients via JSON. Therefore, I wanted to add some struct fields like the following:

type User struct {
    ID          int    `storm:"id,increment"`
    FirstName   string `json:"firstName"`
    LastName    string `json:"lastName"`
    Email       string `storm:"unique" json:"email"`
    Password    string `json:"password"`
    AvatarURL   string `json:"avatarURL"`
    [...]
}

However, when I add the json:"email" tag to the Email field, the storm:"unique" tag gets ignored, and I can create multiple users with the same URL (they essentially overwrite each other, every user created this way has the same ID).

Thanks for your help on this! 😉

asdine commented 6 years ago

@riesinger What version of Storm are you using ? I can't reproduce your issue

riesinger commented 6 years ago

My Gopkg.toml (I am using dep) states the following:

[[constraint]]
  name = "github.com/asdine/storm"
  version = "2.0.0"

However, I also have an issue with dep, so I cannot really tell you which commit I'm on.

EDIT: I just noticed that version 2.0.2 is available, I'll try to update (If dep lets me)

riesinger commented 6 years ago

I just upgraded to the latest version and still have the same issue. Meanwhile, we also switched to the MessagePack Codec, but this still does not provide any better results. Removing the json tag fixes this issue

asdine commented 6 years ago

Can you provide a working example please so i can investigate ?

riesinger commented 6 years ago

Yes, just give me two hours till I'm home:blush:

riesinger commented 6 years ago

So, I just found out that we forgot to recompile after switching to the MessagePack Codec, with this enabled, it works. Nevertheless, I have put a working example up on Github (sorry for messy code, I tried to boil everything down to a bare minimum).

See the repo here

EDIT: The interesting parts are probably in db.go and router.go

asdine commented 6 years ago

@riesinger I can't build the repo because there is a submodule called https://github.com/riesinger/freecloud-client that is private. If it's too much work you can simply provide a main that focuses on what's not working on Storm, no need for an http server i think.

riesinger commented 6 years ago

Oh, I am sorry, just fixed this 😅

asdine commented 6 years ago

This is not an issue with Storm but an issue with the way you are using your JSONDecoder (https://github.com/riesinger/storm_tags_error/blob/master/router.go#L94)

When you save something with autoincrement on, the ID is automatically inserted in your structure. The problem is that you are reusing the same instance of User to unmarshal the payload and to save in the database, over and over again. The first call to Save fills the ID field with 1, then all the of the subsequent calls to Save update the User instead of creating a new one because the ID of the instance is equal to 1, all the next calls update the same User in the database.

Also, the JSONDecoder is not thread safe because you are writing within a shared instance of User with multiple goroutines. You should simply create a user instance everytime you need to unmarshal the payload.

riesinger commented 6 years ago

Okay, thank you for your help! Sorry for having wasted your time 😅 😂

Now that I think about it, it makes sense 😂

asdine commented 6 years ago

No problem, don't hesitate 👍