[24.0] Restore histories API behavior for keys query parameter
#17779
Conversation
Replace the `HistoryMinimal` model with a partial HistoryDetailed model where all fields are optional.
This ensures we return just the fields specified in "keys" when returning a CustomHistory.
When requesting only some keys for a particular history, we don't want to replace the whole history with just a few values.
We now need to explicitly define any field that would be serialized
davelopez
force-pushed
the
24.0_restore_histories_API_behaviour_for_keys_query
branch
from
7630a9f to
95eedff
Compare
| export function isHistorySummary(history: AnyHistory): history is HistorySummary { | ||
| return !("user_id" in history); | ||
| } | ||
|
|
||
| export function isHistorySummaryExtended(history: AnyHistory): history is HistorySummaryExtended { | ||
| return "contents_active" in history && "user_id" in history; | ||
| } |
There was a problem hiding this comment.
How do you feel about adding a constant field in the model that indicates what view we're dealing with ?
The pro is that we don't need helper to figure out the actual type, the con is that we're serializing more than strictly necessary.
There was a problem hiding this comment.
That would simplify things yes, I like the idea, but I didn't consider it because it was changing the API response again 😆 but I can give it a try and see how we feel about it. Should I target this in a follow-up on dev for that change though? Or directly in this PR?
There was a problem hiding this comment.
Let's target dev and mention it at the backend meeting ? We do have a related precedent in model_class which is only used for a similar purpose. Yet another alternative is that we include the model-type in the response header, which doesn't alter the response in the body but that would at least be more explicit than "contents_active" in history && "user_id" in history, which feels like a contract we could easily break accidentally.
|
This is awesome, I like it a lot! |
Supports both HDA and HDCA states.
When there is no content.
|
This one should be ready to review, only the flaky Toolshed tests are failing now. |
|
Thank you @davelopez! |
davelopez
deleted the
24.0_restore_histories_API_behaviour_for_keys_query
branch
Fixes #17726 (comment) and addresses #17726 (comment)
as an example, when requesting
/api/histories?keys=name:Before these changes
[ { "model_class": "History", "id": null, "user_id": null, "name": "Unnamed history" } ]After these changes
[ { "name": "Unnamed history" } ]This PR adds a
partial_modeldecorator discussed in the Pydantic community as a workaround to support partial (all fields optional) models until Python supports this concept natively.The
CustomHistoryViewmodel now replacesHistoryMinimalby using this "partial_model" decorator and including any other fields the client might request. The "allow extra fields" configuration has been removed from the base model and now we need to be explicit about what the history response model can contain.It also includes some refactorings/fixes around client model types since most components assumed the history objects contained all fields.
Finally, I increased the test coverage for using "view" and "keys" query parameters in histories.
How to test the changes?
License