[sql-31] firewalldb: Privacy Mapper schemas, queries and CRUD #1043
Conversation
ellemouton
added
no-changelog
ellemouton
force-pushed
the
sql31
branch
2 times, most recently
from
1c732b8 to
262416f
Compare
| group_id BIGINT NOT NULL REFERENCES sessions(id) ON DELETE CASCADE, | ||
|
|
||
| -- The key of the privacy pair. | ||
| real TEXT NOT NULL, |
There was a problem hiding this comment.
nit:
real is seen as reserved keyword/datatype by many sql syntax highlighting tools (as also seen here in the Github uI), so would potentially choose another name as it gets a bit confusing.
| CREATE TABLE IF NOT EXISTS privacy_pairs ( | ||
| -- The group ID of the session that this privacy pair is associated | ||
| -- with. | ||
| group_id BIGINT NOT NULL REFERENCES sessions(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
does this enforce enough or should we use sessions(group_id) here, since we could use a second session's id in a group which would not be allowed?
There was a problem hiding this comment.
you cannot reference a field that is not the primary key unless it is unique.
We will just need to force this on the CRUD layer
There was a problem hiding this comment.
I figured that, would have been great to enforce this constraint on the db layer, but ok to do it on the CRUD layer, if it's not easily possible
| // | ||
| //nolint:lll | ||
| type SQLPrivacyPairQueries interface { | ||
| SQLSessionQueries |
There was a problem hiding this comment.
could we remove this if we add a foreign key constraint to the group id?
There was a problem hiding this comment.
btw - i do think we can eventually move the check to PrivacyDB method instead - that way we only need to check this once. But for the sake of keeping the interfaces matching the kvdb style for now, let's check for each call.
firewalldb/privacy_mapper_sql.go
Outdated
| _, err = p.queries.GetPseudoForReal(ctx, sqlc.GetPseudoForRealParams{ | ||
| GroupID: groupID, | ||
| Real: real, | ||
| }) | ||
| if err == nil { | ||
| return ErrDuplicateRealValue | ||
| } else if !errors.Is(err, sql.ErrNoRows) { | ||
| return err | ||
| } | ||
|
|
||
| _, err = p.queries.GetRealForPseudo(ctx, sqlc.GetRealForPseudoParams{ | ||
| GroupID: groupID, | ||
| Pseudo: pseudo, | ||
| }) | ||
| if err == nil { | ||
| return ErrDuplicatePseudoValue | ||
| } else if !errors.Is(err, sql.ErrNoRows) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
wouldn't it be better to rely on the db to enforce the constraints on insert? the downside is that we don't perfectly know which constraint was violated, or is there a way to extract that info?
There was a problem hiding this comment.
exactly - I tried that but couldnt get it to work nicely: the reason was cause a unique constraint error here aborts the entire transaction that NewPairs falls under. So then it isnt possible to catch an error outside of this call (but under the same tx) and then continue making other calls with the transaction
Define the privacy mapper schemas and queries.
In this commit, we introduce the SQL impl of the privacy mapper store. The privacy mapper unit tests can now be run against the SQL impl.
Remove the `NewPrivacyMapDB` type alias. It is not needed.
This PR adds the schemas and queries for the privacy mapper DB.
It also adds the SQL implementation of the privacy mapper and plugs it into the unit tests and itests.
Part of #917