CP-52881: add mechanism to filter out event fields #6184
Conversation
We should also add a similar mechanism to get_all_records. A better fix might be to change the type of the field to a ref and move it out to a class of its own and do a DB upgrade. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
| let apply_event_filter = function | ||
| | Rpc.Dict lst as orig -> | ||
| let lst' = maybe_map_fields lst in | ||
| if lst' == lst then |
There was a problem hiding this comment.
Should add a comment that == is indeed what we want.
There was a problem hiding this comment.
Is this check worth doing? It seems to me that the only time this condition will be true is when both lists are empty.
There was a problem hiding this comment.
this needs quality_gate.sh to change in order to pass CI. So all usages of == are actually quite purposeful
There was a problem hiding this comment.
lst and lst' can also match when the filter wasn't applicable to the current dictionary (e.g. this is another object, one that doesn't have any filters).
There was a problem hiding this comment.
There are discussions upstream about adding a separate phys_equal to the stdlib to mark purposeful usages, meanwhile I'll update the quality gate and comment.
There was a problem hiding this comment.
lst and lst' can also match when the filter wasn't applicable to the current dictionary
I don't see how this could possibly be true.
let[@tail_mod_cons] rec maybe_map_fields = function
| [] ->
[]
| (key, _) :: tl when Xapi_globs.StringSet.mem key !Xapi_globs.event_filter ->
(key, omitted) :: (maybe_map_fields [@tailcall]) tl
| hd :: tl ->
hd :: (maybe_map_fields [@tailcall]) tl
(* ... *)
| Rpc.Dict lst as orig ->
let lst' = maybe_map_fields lst in
if lst' == lst thenThe function maybe_map_fields is constructing a new list every time, comparison with == will yield false (unless the input is []). This is copying the list, it can't be physically equal afterwards.
Please correct me if there's something I'm missing.
There was a problem hiding this comment.
Indeed, the physeuqal optimization is missing from maybe_map_fields, and without it this one is pointless.
|
|
||
| let omitted = Rpc.Null | ||
|
|
||
| let[@tail_mod_cons] rec maybe_map_fields = function |
|
This feels too much like a hack (it also conflicts with other changes now), so I'm closing this for now. |
5 participants
We should also add a similar mechanism to get_all_records.
A better fix might be to change the type of the field to a ref and move it out to a class of its own and do a DB upgrade.
It might be better to do this at the DB action level when generating
get_record, although we need to ensure we don't lose these values completely: we do need to retain them when saving the XAPI DB.We should really have an Event.from_filtered API too that allows the client to define a field filter.