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.

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

Jump to bottom

Improved MediaField Graphql support #15003

Merged
merged 29 commits into from
Aug 8, 2024
Merged

Improved MediaField Graphql support #15003

merged 29 commits into from
Aug 8, 2024

Conversation

hyzx86
Copy link
Contributor

@hyzx86 hyzx86 commented Jan 6, 2024 •
edited
Loading

To simplify data binding for front-end single-page applications, add file objects

image

@hyzx86 hyzx86 changed the title Add media filenames on MediaFieldGraphqlType Improved MediaField Graphql support Jan 7, 2024
@hyzx86 hyzx86 requested a review from hishamco January 7, 2024 10:47
@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 9, 2024 •
edited
Loading

Hi @hishamco , I adjusted the type structure of the media file fields and increased the collection of file list objects
To facilitate data binding for common file management components such as elment-ui , amis etc..

hyzx86 and others added 2 commits January 10, 2024 10:59
@hyzx86 hyzx86 mentioned this pull request Jan 14, 2024
Closed
@hishamco hishamco requested a review from MikeAlhayek January 14, 2024 21:04
@github-actions github-actions bot mentioned this pull request Feb 1, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

@hishamco
Copy link
Member

hishamco commented Feb 8, 2024

Fix the conflict

@Skrypt
Copy link
Contributor

Skrypt commented Feb 15, 2024 •
edited
Loading

I'm not sure if we should add the full url as it is relative to the current tenant context. Normally, the html or javascript asset you are using should define if you need a relative or absolute path to these files. The GraphQL query becomes opiniated on the URL by generating it while the path could be different from 2 different contexts. Here, what I suggest is that you create a small javascript function that returns the tenantPath instead if you are using this inside a Vue.js app for example.

The JSON structure though that you used is better than what we did. I think it should have been simply:

"attachments": [
{ "filename": "aomolasmd", "path": "asdoas/dmsaod" }
]

And in Razor there is a helper method AssetUrl.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Feb 18, 2024

The JSON structure though that you used is better than what we did. I think it should have been simply:

"attachments": [ { "filename": "aomolasmd", "path": "asdoas/dmsaod" } ]

And in Razor there is a helper method AssetUrl.

@Skrypt , Sorry, I don't quite understand what you mean, you mean that I should use AssetsUrl to convert the path of the Url to the full address containing the http protocol, right? I looked at the implementation of AssetsUrl, and it works the same way as the Media Graphql API does.

But the sad thing is that, as you said, it takes the absolute path relative to the current site, not the absolute path of the tenant,
On this point, I do make compromises in my application

@Skrypt
Copy link
Contributor

Skrypt commented Feb 23, 2024

What I mean is that adding the absolute URL to the GraphQL query doesn't make sense because it is redundant as it will always return the same protocol//domain/basepath for each of these medias. What you want is to get these from other services and use it in your javascript application instead.

Also, adding this redundant path to the Query also makes the data size to be bigger for nothing.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Feb 24, 2024 •
edited
Loading

What I mean is that adding the absolute URL to the GraphQL query doesn't make sense because it is redundant as it will always return the same protocol//domain/basepath for each of these medias. What you want is to get these from other services and use it in your javascript application instead.

Also, adding this redundant path to the Query also makes the data size to be bigger for nothing.

No, no, because the media files may be stored on Azure or some other server that is not consistent with the current server path, I think it is necessary to return the full path here.
This should be useful when our media files are saved in azure or other non-local storage
Maybe we could implement a directive for the url that lets the user choose whether or not to include the full path

The current url format is strange: /tenantRoot/media/XXX.JPG

There is no problem using this path in mvc scenarios, but if you need to specify the server address in separate clients,
But we've saved one for example
serviceRoot: https://test.oc.com/tenantRoot
We can use serviceRoot for api root or oidc's authorization center address

At this point we also need to save another media file root path?
mediaRoot: https://test.oc.com/

It is currently used in my client application

// Note that path is used here, not url
var imgUrl = serviceRoot+'/media'+path;

@Skrypt
Copy link
Contributor

Skrypt commented Feb 28, 2024

You then need 2 things.

Inject the ShellSettings like this in your Layout.cshtml file to always have the tenantId available for your javascript applications.

@using OrchardCore.Environment.Shell
@inject ShellSettings ShellSettings
@{
    var tenantId = ShellSettings.TenantId;
}

<body data-tenant-id="@tenantId">

Inject your fileProvider service and with the same technique add for example a "data-media-base-host" attribute that you will be able to use.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Feb 28, 2024

Hi @Skrypt ，

I'm talking about the SPA /mobile application usage scenario where we're calling the oc api remotely, not just pages in the oc site's domain

@Skrypt
Copy link
Contributor

Skrypt commented Feb 28, 2024 •
edited
Loading

Then you/we can write an ApiController that will return these. That would really make more sense as also this tenant id will be required if you use something like NSwag studio to generate your proxy client for consuming these API's. It always needs a tenant id context.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Feb 28, 2024

I see, you mean is I must update the url field
from
/{tenantName}/{mediaRoot}/folder1/test.jpg (this is result of mediaFileStore.MapPathToPublicUrl(path))
to
/{mediaRoot}/folder1/test.jpg

right?

this is now oc's url field format
image

@github-actions github-actions bot added the stale label Jul 2, 2024
@github-actions github-actions bot removed the stale label Jul 2, 2024
@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 24, 2024 •
edited
Loading

Dears @Skrypt @Piedone , Sorry I'm a bit confused about this PR how else do I need to adjust it~~

Piedone
Piedone requested changes Jul 24, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone 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 adds support to retrieve the public URL of a Media file. I argued above that this is good and also necessary. I don't think that aspect needs to change.

src/OrchardCore.Modules/OrchardCore.Media/GraphQL/MediaFieldQueryObjectType.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Media/GraphQL/MediaFieldQueryObjectType.cs
return x.Page(x.Source.Paths);
});

Field<ListGraphType<StringGraphType>, IEnumerable<string>>("fileNames")
.Description("the media fileNames")
.Description("the media file names")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this now redundant and can be deprecated? (But kept for now for backward compatibility.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that. It would be great if we can remove the old redundant fields. It's a breaking change, but we already have lot's of them, so why not take this as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think it's a good time to remove them in 2.0.
@Skrypt what do you think? Or remove them in a new PR?

@Piedone
Copy link
Member

Piedone commented Jul 29, 2024

When you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

@sebastienros sebastienros merged commit 1f00245 into OrchardCMS:main Aug 8, 2024
5 checks passed
This was referenced Aug 9, 2024
Closed
Merged
@hyzx86 hyzx86 deleted the mediaFileNames branch August 9, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvkries gvkries gvkries left review comments

@Piedone Piedone Piedone requested changes

@MikeAlhayek MikeAlhayek MikeAlhayek approved these changes

@hishamco hishamco hishamco approved these changes

@sebastienros sebastienros Awaiting requested review from sebastienros

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hyzx86 @hishamco @Skrypt @sebastienros @Piedone @MikeAlhayek @gvkries @lampersky