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

Adds request-direct-access-url file name option #197

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

canpan14
Copy link

In support of this pull request Alfresco/alfresco-community-repo#2065

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2023

CLA assistant check
All committers have signed the CLA.

@@ -10683,6 +10701,9 @@ definitions:
attachment:
type: boolean
description: URL type (embedded/attachment).
fileName:
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd usually have the items from the "Create" entity in the response too - i.e. DirectAccessUrl below. I don't think this is explicitly called out in the v1 API guidelines though.

I've added our architect team to review too, since they usually review additions to the v1 API.

Copy link
Author

@canpan14 canpan14 Jul 14, 2023

Choose a reason for hiding this comment

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

I could add it to the DirectAccessUrl class in alfresco-community-repo, but it looks like the current object structure is fed in from the content store itself. Not exactly sure where that code is or how dangerous it is to alter.
I could make a DirectAccessUrlResponse class for the purposes of extending and adding on the additional field.
It's not hard to return the field, I just want to keep things in line with your expectations for the code base as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tpage-alfresco is correct, please make sure to add the file name to the response too.
In terms of implementation, you have already modified the ContentServiceImpl class, so adding an attribute to DirectAccessUrl (part of our Java public API) is not a problem.

Copy link
Author

Choose a reason for hiding this comment

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

@jamalkm The issue is the call to the content store serialized directly into DirectAccessUrl. While the serialization might still work with an extra field added, I worry it might start to diverge from representing the data object actually returned from the database vs. what we are returning from the API.

DirectAccessUrl directAccessUrl = null;
if (store.isContentDirectUrlEnabled())
{
    try
    {
        // referencing this
        directAccessUrl = store.requestContentDirectUrl(contentUrl, attachment, fileName, contentMimetype, validFor);
    }
    catch (UnsupportedOperationException ex)
    {
        // expected exception
    }
}
return directAccessUrl;

I can still open a PR on it though

Copy link
Author

@canpan14 canpan14 Jul 20, 2023

Choose a reason for hiding this comment

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

Alfresco/alfresco-community-repo#2081
Let me know if this is what you were looking for.

Copy link
Contributor

@jamalkm jamalkm left a comment

Choose a reason for hiding this comment

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

The PR looks good, just filename needs to be added to the response too.

@@ -10683,6 +10701,9 @@ definitions:
attachment:
type: boolean
description: URL type (embedded/attachment).
fileName:
Copy link
Contributor

Choose a reason for hiding this comment

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

@tpage-alfresco is correct, please make sure to add the file name to the response too.
In terms of implementation, you have already modified the ContentServiceImpl class, so adding an attribute to DirectAccessUrl (part of our Java public API) is not a problem.

@tpage-alfresco
Copy link
Member

Argh - sorry. I thought this had already been approved. I merged the main PR earlier today. Shall I revert the main PR back to a branch, or is everyone happy to update the response in a second PR?

@jamalkm
Copy link
Contributor

jamalkm commented Jul 20, 2023

Argh - sorry. I thought this had already been approved. I merged the main PR earlier today. Shall I revert the main PR back to a branch, or is everyone happy to update the response in a second PR?

I'm in favour of a second PR.

@canpan14
Copy link
Author

Second PR to add file name to the response object Alfresco/alfresco-community-repo#2081

@canpan14
Copy link
Author

@jamalkm I added fileName to the response object in the api specs and noticed that expiryTime was improperly listed in the response object as 'expiresAt` so I fixed that as well.

@canpan14 canpan14 requested a review from jamalkm July 21, 2023 14:37
@canpan14 canpan14 requested a review from jamalkm July 22, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants