Skip to content

Add set_partial_override method to the API #668

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 9 commits into
base: master
Choose a base branch
from
Draft

Add set_partial_override method to the API #668

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
693bf28
Add update_partial_overrides method to the API
magdakwiecien Jul 9, 2024
bc9f997
Revert temp package.json change
magdakwiecien Jul 9, 2024
1dd1f79
Update method name and test
magdakwiecien Jul 10, 2024
4bf905a
Lock jsdoc version to fix deps lint error
magdakwiecien Jul 10, 2024
ffc3e5b
fix: Rename method to set_partial_override
magdakwiecien Jul 15, 2024
c2d718b
fix: add new API status codes
mckomo-cl Jul 22, 2024
7e29d28
feat: add invalidate option to delete_derived_resources method
mckomo-cl Aug 1, 2024
5dba0f6
fix: make override params optional
mckomo-cl Sep 11, 2024
37e61ef
fix: open types for UpdatePartialOverridesOptions
mckomo-cl Oct 23, 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
5 changes: 5 additions & 0 deletions lib/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,3 +692,8 @@ exports.update_metadata_rule = function update_metadata_rule(field_external_id,
exports.delete_metadata_rule = function delete_metadata_rule(field_external_id, callback, options = {}) {
return call_api('delete', ['metadata_rules', field_external_id], {}, callback, options);
};

exports.update_partial_overrides = function update_partial_overrides(asset_id, callback, params = {}) {
Copy link

Choose a reason for hiding this comment

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

I would change name to upsert as it does exactly that create or update. It's a unique operation for our API so there is no convention yet for similar operations.

Copy link

Choose a reason for hiding this comment

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

upsert term comes from the database world and not from API.
See this discussion in stack overflow:
https://stackoverflow.com/questions/18470588/in-rest-is-post-or-put-best-suited-for-upsert-operation

Copy link

Choose a reason for hiding this comment

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

You are correct, upsert is not an HTTP API-connotated word but for the sake of good DevEx using that name is better than update where there is no create method.

That being said, it's just my suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding of the whole partial overrides thingy, this method will update a resource with partial overrides (and this might be operation that creates an override, or overrides an existing override).

What do you think about the set_partial_overrides name? It's more human-like language than upsert 😉 , and it still might express the nature of this operation better than update.

Copy link

Choose a reason for hiding this comment

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

I just noticed another thing. The final method name should indicate a singular partial override, as you send a payload of only one partial override.

set_partial_override also sounds good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as you send a payload of only one partial override

Oh, I thought there can be multiple overrides provided.
If that's not the case, why overrides in API payload is an array?

{
        transformation_prefix: string;
        asset_override_uri: string;
        overrides: Array<{
            action: 'gen_fill';
            params: {
                seed: string;
                prompt: string;
                ignore_foreground: boolean;
            }
        }>;
    }

Copy link

@mckomo-cl mckomo-cl Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the future, you will be able to provide overrides to many actions, not just gen_fill.

E.g. consider w_100/e_isolate,b_gen_fill,.... The correct payload for it would be:

{
   "transformation_prefix":"w_100/e_isolate,b_gen_fill,...",
   "asset_override_uri":"http://...",
   "overrides":[
      {
         "action":"gen_fill",
         "params":{
            "seed":"...",
            "prompt":"..."
         }
      },
      {
         "action":"isolate",
         "params":{
            "seed":"..."
         }
      }
   ]
}

You override one transformation prefix with one override snapshot, but the snapshot is result of override of two actions (gen_fill and e_isolate).

Copy link

Choose a reason for hiding this comment

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

CC @mikeys ^

const uri = ["resources", asset_id, "partial_overrides"];
return call_api("put", uri, params, callback, {content_type: 'json'});
};
5 changes: 4 additions & 1 deletion lib/v2/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@


const api = require('../api');
const v1_adapters = require('../utils').v1_adapters;

Expand Down Expand Up @@ -74,5 +76,6 @@ v1_adapters(exports, api, {
add_related_assets: 2,
add_related_assets_by_asset_id: 2,
delete_related_assets: 2,
delete_related_assets_by_asset_id: 2
delete_related_assets_by_asset_id: 2,
update_partial_overrides: 1
});
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"eslint-plugin-import": "^2.20.2",
"expect.js": "0.3.x",
"glob": "^7.1.6",
"jsdoc": "^3.5.5",
"jsdoc": "3.5.5",
"jsdom": "^9.12.0",
"jsdom-global": "2.1.1",
"mocha": "^6.2.3",
Expand Down
20 changes: 20 additions & 0 deletions test/integration/api/admin/api_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1599,4 +1599,24 @@ describe("api", function () {
}
});
});
describe('update_partial_overrides', () => {
it("should call the PUT /resources/:asset_id/partial_overrides endpoint", async () => {
this.timeout(TIMEOUT.LONG);
await retry(async function () {
return helper.provideMockObjects((mockXHR, writeSpy, requestSpy) => {
cloudinary.v2.api.update_partial_overrides('ASSET_ID_MOCK', {
transformation_prefix: 'tx_prefix',
asset_override_uri: 'snapshot_url',
overrides: [{ action: 'gen_fill', params: { seed: 'seed', prompt: 'prompt', ignore_foreground: true } }]
});
sinon.assert.calledWith(writeSpy, sinon.match(helper.apiJsonParamMatcher('transformation_prefix', 'tx_prefix')));
sinon.assert.calledWith(writeSpy, sinon.match(helper.apiJsonParamMatcher('asset_override_uri', 'snapshot_url')));
return sinon.assert.calledWith(requestSpy, sinon.match({
pathname: sinon.match("resources/ASSET_ID_MOCK/partial_overrides"),
method: sinon.match("PUT")
}));
});
});
});
})
});
17 changes: 17 additions & 0 deletions types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,19 @@ declare module 'cloudinary' {
[futureKey: string]: any;
}

export interface UpdatePartialOverridesOptions {
transformation_prefix: string;
asset_override_uri: string;
overrides: Array<{
action: 'gen_fill';
params: {
seed: string;
prompt: string;
ignore_foreground: boolean;
}
}>;
}

export interface UploadApiOptions {
access_mode?: AccessMode;
allowed_formats?: Array<VideoFormat> | Array<ImageFormat>;
Expand Down Expand Up @@ -1215,6 +1228,10 @@ declare module 'cloudinary' {

function restore_metadata_field_datasource(field_external_id: string, entries_external_id: string[], callback?: ResponseCallback): Promise<DatasourceChange>;

function update_partial_overrides(public_id: string, options: UpdatePartialOverridesOptions, callback?: ResponseCallback): Promise<any>;

function update_partial_overrides(public_id: string, callback?: ResponseCallback): Promise<any>;

/****************************** Structured Metadata Rules API V2 Methods *************************************/
function add_metadata_rule(rule: MetadataRule, options?: AdminApiOptions, callback?: ResponseCallback): Promise<MetadataRuleResponse>;

Expand Down