registration entry federated trust domains support #563
Conversation
azdagron
requested review from
amartinezfayo,
evan2645,
MarcosDY,
martincapello and
walmav
as code owners
| @@ -9,7 +9,7 @@ import ( | |||
|
|
|||
| const ( | |||
| // version of the database in the code | |||
| codeVersion = 1 | |||
| codeVersion = 2 | |||
|
|
||
| func migrateToV2(tx *gorm.DB) error { | ||
| // add the federated_with column | ||
| if err := tx.AutoMigrate(&RegisteredEntry{}).Error; err != nil { |
There was a problem hiding this comment.
Should we continue use of AutoMigrate here, or do it by hand? The next time we change RegisteredEntry model, will the migrateToV2 method still work as expected?
There was a problem hiding this comment.
I guess it depends on "work as expected" :) It will bring the table up to the latest schema, which could possible break migrations that come after that (that hopefully the unit tests would catch). I was hoping GORM exported AddColumn functionality, but it does not. If needed, we could crib some code from GORM (all of the code that does the "add column" is exported, i think).
There was a problem hiding this comment.
Heh... I was thinking to use tx.Exec again as we did in migrateToV1? Is that a bad idea?
There was a problem hiding this comment.
The only reason I was ok with that exec was because it only used the table and column names, which as far as I understand it, don't change with the dialect.
This code would be altering the table and adding a column with a dialect specific column type, so it would be better to use their code (we could patch up migrateV1 as well, to be consistent).
| } | ||
| return responseEntries, nil | ||
| } | ||
|
|
||
| func (ds *sqlPlugin) convertEntry(regEntry *RegisteredEntry) (*common.RegistrationEntry, error) { |
There was a problem hiding this comment.
nit: the name of this function could be more specific, it is hard to tell what we're converting to/from without referencing the declaration... perhaps modelToEntry or something similar would be clearer?
There was a problem hiding this comment.
i was keeping the naming consistent with the other function (convertEntries). I will rename them both.
| if s == "" { | ||
| return nil | ||
| } | ||
| return strings.Split(s, ",") |
There was a problem hiding this comment.
If I recall correctly, the reason for doing this was a result of difficulty with many-to-many mappings under gorm, and a desire to look up entries by providing a federated trust domain... I don't think we'd get the latter with this, right? What do we get by doing this that we wouldn't get with something more traditional like FederatesWith []string ?
There was a problem hiding this comment.
FederatesWith []string is not a valid field type for use with GORM (you need a slice of models). We could model a one-to-many by creating a new struct for the federated id and doing a slice of those. Or we can do many-to-many with full back-reference support (and avoid AutoMigrate because it fails trying to create the join table twice). Or we can do many-to-many without back-reference support. The "query by federated trust id" is an optimization could be useful. It would certainly be more performant with a many-to-many relationship set up, but nothing precludes doing a LIKE expression against the currently implemented federates_with column. We could also press for GORM to fix the auto migrate bug....
There was a problem hiding this comment.
Welp. Consider me embarrassed. The many2many auto migrate bug is fixed in the latest GORM. I'll go back to many2many and update the gorm package.
azdagron
force-pushed
the
reg-entry-federates-with-support
branch
4 times, most recently
from
787a83a to
eabe5ce
Compare
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
force-pushed
the
reg-entry-federates-with-support
branch
from
50251a6 to
c28c439
Compare
Adds support to the datastore for storing federated trust domain ids for each registration entry.