Skip to content

feat(odsp-client): Add etag file conversion API attach options #25804

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat(odsp-client): Add etag file conversion API attach options #25804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e7f000
Add etag file conversion API attach options
znewton Oct 21, 2025
bb32f62
Merge branch 'main' into odsp-dedupe-etag-conversion
znewton Nov 3, 2025
83e52d2
API report
znewton Nov 3, 2025
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
2 changes: 2 additions & 0 deletions packages/service-clients/odsp-client/api-report/odsp-client.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ export interface OdspConnectionConfig {

// @beta (undocumented)
export interface OdspContainerAttachProps {
eTag?: string;
fileName: string | undefined;
filePath: string | undefined;
itemId?: string;
}

// @beta
Expand Down
2 changes: 2 additions & 0 deletions packages/service-clients/odsp-client/api-report/odsp-client.beta.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ export interface OdspConnectionConfig {

// @beta (undocumented)
export interface OdspContainerAttachProps {
eTag?: string;
fileName: string | undefined;
filePath: string | undefined;
itemId?: string;
}

// @beta
Expand Down
12 changes: 12 additions & 0 deletions packages/service-clients/odsp-client/src/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,18 @@ export interface OdspContainerAttachProps {
* The file name of the Fluid file. If undefined, the file is named with a GUID.
*/
fileName: string | undefined;

/**
* The ID of the item (file) to which the container is being attached.
* When combined with eTag, this will trigger a conversion of an existing file to a Fluid file.
Copy link
Contributor

@Josmithr Josmithr Nov 10, 2025

Choose a reason for hiding this comment

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

How coupled are itemId and eTag? Would it make sense to combine them into a single object property? Or are they really independent?

Copy link
Contributor Author

@znewton znewton Nov 11, 2025

Choose a reason for hiding this comment

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

The feature only works with both, so I can couple them; however, their the same "level" of metadata as the other properties here, so it would be a bit weird to nest them I think

Copy link
Contributor

@Josmithr Josmithr Nov 11, 2025

Choose a reason for hiding this comment

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

Oh, do these correspond to external ODSP properties or something? If so, should we document that here?

Copy link
Contributor Author

@znewton znewton Nov 11, 2025

Choose a reason for hiding this comment

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

Yeah this is information from the external (not-fluid) ODSP file

Copy link
Contributor

@Josmithr Josmithr Nov 11, 2025

Choose a reason for hiding this comment

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

Are there public docs we can point to from the interface-level comment? Might be nice to make that relationship explicit in the docs.

*/
itemId?: string;
Copy link
Contributor

@Josmithr Josmithr Nov 10, 2025

Choose a reason for hiding this comment

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

Nit: we should probably type these with | undefined to match the others. With our current TypeScript configurations, it doesn't make a big difference. But longer term we want to enable exactOptionalPropertyTypes, at which point the distinction matters (and these should all probably align).


/**
* Optional eTag to use when attaching the container.
* If provided, the container will
Copy link
Contributor

@Josmithr Josmithr Nov 10, 2025

Choose a reason for hiding this comment

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

Incomplete sentence here, FYI

*/
eTag?: string;
}

/**
Expand Down
25 changes: 19 additions & 6 deletions packages/service-clients/odsp-client/src/odspClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,25 @@ export class OdspClient {
const attach = async (
odspProps?: ContainerAttachProps<OdspContainerAttachProps>,
): Promise<string> => {
const createNewRequest: IRequest = createOdspCreateContainerRequest(
connection.siteUrl,
connection.driveId,
odspProps?.filePath ?? "",
odspProps?.fileName ?? uuid(),
);
const createNewRequest: IRequest =
odspProps?.eTag !== undefined && odspProps?.itemId !== undefined
? {
url: createOdspUrl({
siteUrl: connection.siteUrl,
driveId: connection.driveId,
itemId: odspProps.itemId,
dataStorePath: "",
}),
headers: {
eTag: odspProps.eTag,
},
}
: createOdspCreateContainerRequest(
connection.siteUrl,
connection.driveId,
odspProps?.filePath ?? "",
odspProps?.fileName ?? uuid(),
);
if (container.attachState !== AttachState.Detached) {
throw new Error("Cannot attach container. Container is not in detached state");
}
Expand Down
Loading