OrchardCMS
Improved MediaField Graphql support
To simplify data binding for front-end single-page applications, add file objects
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..
This pull request has merge conflicts. Please resolve those before requesting a review.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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
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.
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;
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.
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
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.
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
Let's add @sebastienros to this discussion to see what he thinks about it. Because, originally, from memory, it was designed to not include any specific folders for a reason.
Now, if we need to expose some configurations/folders from an ApiController then let's decide.
I believe the "path only" is only for storing the field content in the db. It's ok to expose more detailed info in graphql. However I am not sure exposing the current 'url' makes sense because this should use the actual origin domain/path, even if there was a proxy. So I'd rather return only the path and let the client use relative links to these files. Unless we use the same technique we use when we build absolute urls (including scheme, domain, ... from the client, as there could be a proxy in between the client and server).
I'm currently refactoring the Media Gallery in OC and I'm simply passing a baseHost param to the media gallery when I want to use it as standalone. I'm just appending the baseHost to the file urls when the app receives it. That also goes for all the TSClient proxy client calls.
https://github.com/OrchardCMS/OrchardCore/pull/14256
Works fine to me in both scenarios (Razor vs standalone).
And, this way I do not need to alter any GraphQL endpoint or any changes required with the ApiController except that I strongly typed the ApiController returns for the TSClient. Also, do not send duplicate data on these ApiController requests for nothing.
Maybe the changes are not actually in the PR branch linked but that's what I'm currently working on for my current job. And on my current branch, that's how it works.
Here for this PR the context though is from a ContentType Media Picker Field. So, what I agree on is the fact that the JSON returned from the GraphQL endpoint could be formatted a little better like you did.
I haven't seen the Media path format of AzureMediaStore module, if we use AzureMediaStore ,
will this url field show the full path?
I've not tried yet but I'm guessing you could get it from there:
Yes, I see what you mean, but it's not something that should be addressed in this Issue, I think it should be put into a separate PR, what do you think? 😊
After all, we haven't introduced any disruptive changes yet, and if we modify the path it will need to introduce more changes
Note that Media files may be stored behind a CDN too, which can have a different host (MediaOptions.CdnBaseUrl) than the site's. So, returning the full public URL is necessary (and the path, as it is in this PR already)
Yeah, but you are growing the size of the payload of these GraphQL queries for something that can be retrieved differently and also only once per page request. If you are using a CDN then you have a setting for that to retrieve the base path.
My point is that if you want to return the public URL of a Media (and not just the Media-based path), then that should be an absolute URL, because you can't assume the host will be the same as the site's. Otherwise, I wouldn't return it at all, since it may be incorrect, it'd be an unreliable API.
But with GraphQL you can select the fields anyway, so you'll get the public URL only if you actually want it, so I don't really see the issue (like you'll get a couple of characters more in the non-CDN case, but is that really something we should care about?).
See Sebastien's answer. I'm not adding more to the subject. I will let you guys decide. You know my opinion. Let's cut the debate. :smile:
I also see no reason not to include the public URL just like any other field, the user can still decide to get the relative path and append their host manually. But they can also retrieve the full url if they want to. However, if we add the full URL here, do we also need the URL in other places (I.e, the item-display URL for the content item?) Alternatively, we can have another object that we can fetch that would give us site info like (media URL, Site Base URL) which then the user can use that info to build the full URL on their side and save payload bandwidth.
Actually with GraphQL you can omit to retrieve that field that has the full absolute url. So, if we are moving forward with this change I don't care much since I will never use it simply. But that's not my point. My point is that this absolute path can be irrelevant if you are behind a proxy for example. So, this is the client app itself that should define which http://domain/ portion of the URL it should use.
i.e. Am I behind a proxy ... use http://proxieddomain else fetch the base url from an OC API request.
Then do a GraphQL query to get that base path...
If the OC app is behind a proxy, then it needs to know about that proxy (and that needs to be properly configured either for CDN Base Url, or the host header be forwarded and the app configured to accept it; otherwise, you couldn't reach the API either). So, the absolute URL should still be correct.
If the OC app is behind a proxy, then it needs to know about that proxy (and that needs to be properly configured either for CDN Base Url, or the host header be forwarded and the app configured to accept it; otherwise, you couldn't reach the API either). So, the absolute URL should still be correct.
To be honest, I tried the CDN Base Url setting option , it doesn't seem to work on the Media module. I haven't investigated why, but I see it's in the code
Which Media module? The updated vue app I'm working on?
I'm talking about graphql, as in my screenshot above, and I actually configured cdnbaseurl , according to the current code, the url field should display the full path, including cdnbaseurl
This pull request has merge conflicts. Please resolve those before requesting a review.
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.