-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Description
We are inconsistent about when Services use nested dictionaries versus when they use Pydantic model instances. These are easy to convert between via nested_dict = model.model_dump() and model = schema(**nested_dict), but they have different interfaces. This means that the Service method and the ConnectionManager method that is created by services.core.utils.generate_connection_manager don't necessarily have the same signature, and don't necessarily use the same types for input and output. Probably the worst problem is that the input signature of the Service method is just a single dictionary, which is bad for readability.
The Service method receives a dictionary, and can return either a dictionary or a Pydantic model.
The ConnectionManager method can receive a dictionary, a Pydantic model, or **kwargs. (I do think having all of these as an option is a good thing!) It converts any of these to a Pydantic model then to a nested dict which can be JSON serialised for passing via FastAPI. It receives a dictionary which it then converts to Pydantic model. [Though it is possible to turn these conversions to Pydantic models off via the validate_input and validate_output arguments to the ConnectionManager method, that isn't normal use.]
For me the most readable would be if the Service methods had named arguments, so were effectively passed **model.model_dump() (rather than just model.model_dump() as currently). This would require some kind of wrapping of the method in the Service class, presumably in add_endpoint.
I could also be convinced by the same change but going to a Pydantic model instead, so it is possible (though not required) for both the ConnectionManager and Service methods to use Pydantic models as both input and output.
I think this is impossible to do without being a breaking change, though backwards compatibility could be maintained by adding an argument to add_endpoint (though I think changing the default at some point would be good).
There is a secondary related issue where it would be better if the Services we have in the package - there are many in the Cluster module, probably others elsewhere - consistently returned Pydantic models, rather than just dictionaries. This doesn't make any difference in practice, it's just more readable. In some cases, however, this is difficult, because some of the methods currently return Redis versions of UnifiedMindtraceDocument subclasses - these are still BaseModel subclasses so JSON serialise fine but fail Pydantic validation pre-serialisation.
Acceptance Criteria
- Clear signposting of any breaking changes
Priority
Low (cleanup or optional enhancement)
Estimated Effort (Story Points)
2 (1 day)