Metric context query #638
Metric context query #638
Conversation
is it a mistake here -> |
| func (f MetricFixtures) CreateMetric(ctx context.Context, metric *models.Metric) (*models.Metric, error) { | ||
| if metric.Context != nil { | ||
| if err := f.baseFixtures.db.WithContext(ctx).FirstOrCreate(&metric.Context).Error; err != nil { | ||
| return nil, eris.Wrap(err, "error creating metric context") | ||
| } | ||
| metric.ContextID = &metric.Context.ID | ||
| } | ||
| if err := f.baseFixtures.db.WithContext(ctx).Create(metric).Error; err != nil { | ||
| return nil, eris.Wrap(err, "error creating test metric") | ||
| return nil, eris.Wrap(err, "error creating metric") | ||
| } | ||
| return metric, nil | ||
| } |
There was a problem hiding this comment.
can you please check this moment -> https://gorm.io/docs/create.html#Create-With-Associations. amybe we even don't need to create twice and gorm can do all the magic under the hood.
There was a problem hiding this comment.
Gorm is not doing the magic we need -- it could perhaps become a BeforeCreate hook on the Metric and LatestMetric model?
| if metric.Context != nil { | ||
| if err := f.baseFixtures.db.WithContext(ctx).FirstOrCreate(&metric.Context).Error; err != nil { | ||
| return nil, eris.Wrap(err, "error creating latest metric context") | ||
| } | ||
| metric.ContextID = &metric.Context.ID | ||
| } | ||
| if err := f.baseFixtures.db.WithContext(ctx).Create(metric).Error; err != nil { | ||
| return nil, eris.Wrap(err, "error creating test latest metric") | ||
| return nil, eris.Wrap(err, "error creating latest metric") | ||
| } |
There was a problem hiding this comment.
the same moment here -> maybe we can just create everything during one gorm call?
There was a problem hiding this comment.
gorm is not able to do "FirstOrCreate" on the association by magic, but I think we could move to Metric.BeforeCreate hook if that's preferable (?)
There was a problem hiding this comment.
that's a bit strange cause they say:
When creating some data with associations, if its associations value is not zero-value, those associations will be upserted, and its Hooks methods will be invoked.
``` - upsert. if no, then ok.
There was a problem hiding this comment.
my testing did not show that to work in this case, possibly because our FK relationship is inverted (ie, we'd have to create Context and then it could auto create Context.Metrics if we had that association defined).
yes, i couldn't add yet because the endpoint isn't done (separate story) -- I think we might have to revisit when they come together. |
| ).Joins( | ||
| "LEFT JOIN contexts on metrics.context_id = contexts.id", | ||
| ) | ||
| sql, args := repositories.BuildJsonCondition(tx.Dialector.Name(), "contexts.json", metricContext) |
There was a problem hiding this comment.
can't we pass it right into Where?
There was a problem hiding this comment.
Same as above, personally I think what Geoff wrote is more readable...
There was a problem hiding this comment.
also we have to ... the second return val in this approach
Add metric context querying in the repository layer. Metrics are filtered by a map of values, where the key is a json path, and value is what we expect at that path. For instance, given a context struct like
{ "key1": "value1", "key2": { "key3": "value3"}}, the context could be queried withmap[string]string{"key1": "value1", "key2.key3", "value3"}. This seems to fit with the query inputs we expect from Aim client, such asmetric.context.key1 = value1.Fixes #358
Fixes #226