ENG-48723: Filter builder fix and object store changes #230
Conversation
| .build()); | ||
| } | ||
|
|
||
| public ContextualConfigObject<T> upsertObject(RequestContext context, T data, Filter filter) { |
There was a problem hiding this comment.
I thought on this again.
We don't need new abstraction we can just overload the method in base store reasons:
- The Filter is a generic class can be used directly without a generic type.
- It was difficult to convert different type of request (like update, or get ) under a single abstract method which builds filter
- we can build the filter in Impl class and pass the filter.
- We can have overload on other get methods and this seems simpler for impl class also
There was a problem hiding this comment.
I think that's fine, with a few caveats:
we can build the filter in Impl class and pass the filter.
Let's make the update w/ filter method protected then. It's on the child class to expose a method that takes in its own filter input and converts to the generic type if that's a relevant use case.
I also think it could be worthwhile to provide some filter building utilities for easily going from a first class proto filter object to a generic one, but we can park that and implement it a few times before pulling out commonalities.
There was a problem hiding this comment.
i get an idea but not exactly.
Let me try and push few commits for reference
There was a problem hiding this comment.
with a few caveats
what are the limitations you see? i will try to see if i can handle them
There was a problem hiding this comment.
what are the limitations you see? i will try to see if i can handle them
Just the points from the other comments. Mainly that
- we should avoid writing custom conversion code - we can definitely figure something out between the existing converter and/or object mapper.
- the point about making the update w/ filter protected. We can let the impl class deal with building it and expose a typed api. We don't need the impl class exposing the generic filter method without the conversion (i.e. callers to the specific store shouldn't be building a filter)
There was a problem hiding this comment.
Made the above changes
I also think it could be worthwhile to provide some filter building utilities for easily going from a first class proto filter object to a generic one, but we can park that and implement it a few times before pulling out commonalities
on this what type of utilities i can add?
There was a problem hiding this comment.
Conversion looks good. Can you fix the second point - make your new method protected so it doesn't expose it to all callers.
on this what type of utilities i can add?
Let's come back to this once we build a few implementations. But basically what I was thinking is most typed filters are a series of optional (field, primitive) pairs that are used with equality, and then we AND together any provided pairs. I suspect we could automatically build the generic filter with minimal config (e.g. providing a mapping of filter field number to the json path). But anyway as I said, we can revisit in the future only if it seems worthwhile.
There was a problem hiding this comment.
right, i know what can we add but at this stage not have enough sets to build one.
| .build()); | ||
| } | ||
|
|
||
| public ContextualConfigObject<T> upsertObject(RequestContext context, T data, Filter filter) { |
There was a problem hiding this comment.
I think that's fine, with a few caveats:
we can build the filter in Impl class and pass the filter.
Let's make the update w/ filter method protected then. It's on the child class to expose a method that takes in its own filter input and converts to the generic type if that's a relevant use case.
I also think it could be worthwhile to provide some filter building utilities for easily going from a first class proto filter object to a generic one, but we can park that and implement it a few times before pulling out commonalities.
| @@ -106,4 +100,39 @@ private Filter evaluateLeafExpression(RelationalFilter relationalFilter) { | |||
| private String buildConfigFieldPath(String configJsonPath) { | |||
| return String.format("%s.%s", CONFIG_FIELD_NAME, configJsonPath); | |||
| } | |||
|
|
|||
| public Object convertValueToObject(Value value) { | |||
There was a problem hiding this comment.
I wonder if we need these / already have them?
In the doc write flow, we have the implementer convert between proto and value. The proto serializes to a json string which is basically what we want here. In other words:
- does this belong in
ConfigProtoConverterwhere we already have Value->Proto, Proto->Value, Value->String, String->Value methods? - If yes, could we just do Value->String and then toss it in object mapper to convert it like this?
There was a problem hiding this comment.
we have converter. Converting using the above method
| @@ -54,47 +61,34 @@ private Filter evaluateCompositeExpression(LogicalFilter logicalFilter) { | |||
| } | |||
|
|
|||
| private Filter evaluateLeafExpression(RelationalFilter relationalFilter) { | |||
| Object value = this.convertValueToObject(relationalFilter.getValue()); | |||
There was a problem hiding this comment.
So are we sure the Value object doesn't work as-is here? What does doc store do with it?
There was a problem hiding this comment.
yes when i was doing e2e test i see mongo throwing codec not found exception. It does not have serializer/des for the Value class. So i have to explicitly convert it to java object class
There was a problem hiding this comment.
Got it, I had assumed they were stringifying it for some reason (and it stringifies to JSON). In that case, approach above makes sense.
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.