Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[24.0] Restore histories API behavior for keys query parameter #17779

Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ba074bd
Preserve API behavior for "keys"
davelopez Mar 14, 2024
e2ad32d
Revert a7ea52841748437acfb172e11f513eed6f2fcceb
davelopez Mar 15, 2024
947c6da
Do not force serialize model_class when it is Optional
davelopez Mar 17, 2024
c7f0fb0
Use `response_model_exclude_unset` whenever we return `AnyHistoryView`
davelopez Mar 17, 2024
9f11c07
Add API test for show history views/keys
davelopez Mar 17, 2024
1683dfb
Do not mix `await` and `then`
davelopez Mar 17, 2024
6add6cc
Remove global Vue import
davelopez Mar 17, 2024
671b522
Fix side effect when updating parts of a history
davelopez Mar 17, 2024
fbc2d9e
Add TODO doc for partial_model
davelopez Mar 17, 2024
95fb8da
Remove config to allow all extra fields for history models
davelopez Mar 18, 2024
aba671c
Increase test coverage for API queries with view and keys
davelopez Mar 18, 2024
df2e202
Reorder union for proper model matching
davelopez Mar 18, 2024
f92f14e
Add missing optional fields to CustomHistoryView
davelopez Mar 18, 2024
4f97f7e
Update client API schema
davelopez Mar 18, 2024
d5d26b3
Refactor move user-related types to api module
davelopez Mar 18, 2024
b0bccaf
Fix typo
davelopez Mar 18, 2024
178d7b7
Refactor and reuse userOwnsHistory
davelopez Mar 18, 2024
95eedff
Fix typing issue
davelopez Mar 18, 2024
3085b6e
Add HistoryContentStateCounts
davelopez Mar 19, 2024
b7f3c66
Ensure `contents_active` return 0 instead of None
davelopez Mar 19, 2024
df60d8e
Replace deprecated model.dict with model.model_dump
davelopez Mar 19, 2024
1d38546
Fix UpdateHistoryContentsBatchPayload model_config
davelopez Mar 19, 2024
5aa5537
Fix model_config for UpdateHistoryContentsPayload
davelopez Mar 19, 2024
7dd3f71
Remove unused HistoryBase model
davelopez Mar 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions client/src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,19 @@ import { components } from "@/api/schema";
*/
export type HistorySummary = components["schemas"]["HistorySummary"];

export interface HistorySummaryExtended extends HistorySummary {
size: number;
contents_active: components["schemas"]["HistoryActiveContentCounts"];
user_id: string;
}

/**
* Contains additional details about a History.
*/
export type HistoryDetailed = components["schemas"]["HistoryDetailed"];

export type AnyHistory = HistorySummary | HistorySummaryExtended | HistoryDetailed;

/**
* Contains minimal information about a HistoryContentItem.
*/
Expand Down Expand Up @@ -125,3 +133,46 @@ export function hasDetails(entry: DatasetEntry): entry is DatasetDetails {
* Contains dataset metadata information.
*/
export type MetadataFiles = components["schemas"]["MetadataFile"][];

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;
}
Comment on lines +137 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


type QuotaUsageResponse = components["schemas"]["UserQuotaUsage"];

export interface User extends QuotaUsageResponse {
id: string;
email: string;
tags_used: string[];
isAnonymous: false;
is_admin?: boolean;
username?: string;
}

export interface AnonymousUser {
isAnonymous: true;
username?: string;
is_admin?: false;
}

export type GenericUser = User | AnonymousUser;

export function isRegisteredUser(user: User | AnonymousUser | null): user is User {
return !user?.isAnonymous;
}

export function userOwnsHistory(user: User | AnonymousUser | null, history: AnyHistory) {
return (
// Assuming histories without user_id are owned by the current user
(isRegisteredUser(user) && !hasOwner(history)) ||
(isRegisteredUser(user) && hasOwner(history) && user.id === history.user_id)
);
}

function hasOwner(history: AnyHistory): history is HistorySummaryExtended {
return "user_id" in history;
}
Loading
Loading