[#34314] - [4.1] Media web service implementation
- Closed
- 8 Oct 2021
- Medium
- Build: 4.0-dev
- # 34314
- Diff
- pjdevries:media_web_service
User tests: Successful: Unsuccessful:
Joomla-Mono-Vertical-logo-light-background-en.png.base64.txt
Vertical-logo-light-background-en.png.base64.txt
Pull Request for Issue #34224 .
Summary of Changes
My take on a media web service implementation. It relies on com_media's ApiModel to provide the logic. Two service models, MediumModel and MediaModel, provide wrappers around ApiModel to provide an interface as expected by the standard web service base classes.
The service recognizes the following endpoints:
GET {{base_path}}/api/index.php/v1/media
GET {{base_path}}/api/index.php/v1/media/{path}
POST {{base_path}}/api/index.php/v1/media
PATCH {{base_path}}/api/index.php/v1/media/{path}
DELETE {{base_path}}/api/index.php/v1/media/{path}
{path} parameters in the above endpoints represent paths of existing files or folders, relative to the media base folder (images by default). They are used in single item requests instead of the usual numeric item id, since media items don't have id's.
The web service mostly recognizes the same json request parameters as the com_media ApiModel. These are:
path: A file or folder path wherever appropriate. For instance, in case of the list GET request it points to the base folder, from which to retrieve the list of files and/or folders. Or the destination of a file or folder for a POST or PATCH request.
url: Instructs com_media to include a separate url attribute in the resulting file objects. Has no effect on folder objects.
temp: Instructs com_media to include a separate tempUrl attribute in the resulting file objects. I have no idea what the added value for this parameter is. Has no effect on folder objects.
content: Media content in base64 format for POST or PATCH requests.
In places where path is used, it can be prefixed with adapter: to access files managed using that specific adapter. For eaxample: local-images:/banners/osmbanner1.png, dpdropbox-Dropbox:/fo/bar/baz.jpg, etc. If no adapter is specified, local-images is assumed.
These are issues I struggled with during the implementation:
- The web service API heavily relies on the notion that all data comes from a database.
- Media files have no database entries and thus no record id.
LocalAdapter::createFolder()ignores the result ofFile::create(), so the sweb service has no clue if creating a new file or folder succeeded.LocalAdaptersanitizes file names, so creating files or folders with questionable names (according to some) should not be a problem. However, the input PATH filter rejects questionable, but otherwise perfectly valid filenames (see #34279).LocalAdapterfile/folder manipulation methods do not behave very consistently:createFile() and createFolder()return a file or folder basename.move()returns a path (directory + file-/foldername) with leading slash.updateFile(), moveFile() and moveFolder()return nothing.
- Exceptions thrown by underlying code, com_media's
ApiModelin this case, are not handled properly by the default web service logic and result in 500 errors. I encountered similar behaviour before with other web services.
Testing Instructions
Discover and enable the media web service plugin.
Obtain an active Joomla Api Token.
Run the following cURL commands, replacing the following and other, similar place markers:
- [your J4 website] with the name of your website
- [your user token] with the above mentioned Joomla Api Token.
- [your path] with the path of a file or folder to request or manipulate, relative to the base media folder (i.e. images).
Care must be taken that path names comply with the overeager input PATH filter.
The two attached base64 encoded files can be used to test POST and PATCH requests.
GET list
curl --location --request GET 'https://[your J4 website]/api/index.php/v1/media' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]'
GET item
curl --location --request GET 'https://[your J4 website]/api/index.php/v1/media/banners/banner.jpg' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]'
POST new item
curl --location --request POST 'https://[your J4 website]/api/index.php/v1/media' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]' \
--data-raw '{
"path": "joomla_logo/Vertical-logo-light-background-en.png",
"content": "[your base64 content]"
}'
PATCH update existing item
curl --location --request PATCH 'https://[your J4 website]/api/index.php/v1/media/**[your file or folder]**' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]' \
--data-raw '{
"path": "[your new path]",
"content": "[your new base64 content]"
}'
DELETE remove existing item
curl --location --request DELETE 'https://[your J4 website]/api/index.php/v1/media/[your file or folder]' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]'
Documentation Changes Required
There is no documentation yet.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- 
| Category | ⇒ | Administration Language & Strings Front End Plugins |
ouch i've omitted the filename from the body path....i'll retry....
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34314.
@alikon and/or @wilsonge I discovered a small issue with Media Manager's path settings in combination with a default FileSystem - Local configuration (see #34358). This also reflects on this media web service implementation. I have a fix, but what to do: first merge this PR and then add a separate PR for the fix or add the fix to this PR?
as long as this pr it'only tested by me ... than better you'll complete in this pr
| Labels |
Added:
?
?
?
|
||
as long as this pr it'only tested by me ... than better you'll complete in this pr
Consider it done.
sure ...
cannot help too much i'm a complete com_media dumb
sure ...
cannot help too much i'm a complete com_media dumb?
Me too, as the experts looking at my implementation can confirm undoubtedly :D
The good news is I hardly had to change anything, because the current implementation already supports multiple adapters. To clarify, I added this to the PR description:
In places where path is used, it can be prefixed with adapter: to access files managed using that specific adapter. For eaxample: local-images:/banners/osmbanner1.png, dpdropbox-Dropbox:/fo/bar/baz.jpg, etc. If no adapter is specified, local-images is assumed.
The only thing I had to change, was the 'PATH' filter to clean input parameters, because it doesn't accept completely valid paths' (see also #34279).
| Labels |
Added:
?
Removed: ? |
||
| Labels |
Added:
?
Removed: ? |
||
I have tested this item
All endpoints except patch work.
- Successful PATCH including path and content returns 406 with response body {"errors":[{"title":"Not Acceptable"}]}.
- DELETE should return 204 No Content.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34314.
| Labels |
Added:
?
Removed: ? |
||
I have tested this item
? unsuccessfully on f9793fcAll endpoints except patch work.
* Successful PATCH including path and content returns 406 with response body {"errors":[{"title":"Not Acceptable"}]}. * DELETE should return 204 No Content.
The mentioned issues should be solved now. Exception handling has been improved and more useful feed back is provided when exceptions occur, both for the above mentioned PATCH request and for other requests. DELETE now returns status code 204, but still returns a literal 1 as discussed in #34269.
I don't see any literal 1's being returned from the webservice here in postman (but it has deleted the files)
One thing obviously from looking at this PR for a few minutes btw. Is that we can't have an ID of 0 for all the items. JSON API spec states Within a given API, each resource object’s type and id pair MUST identify a single, unique resource. Probably for what we are doing here that means the id for a media item should be the same as the path attribute (and potentially remove the path attribute).
| Labels |
Added:
?
Removed: ? |
||
| Labels |
Added:
?
Removed: ? |
||
I don't see any literal 1's being returned from the webservice here in postman (but it has deleted the files)
My mistake. I never looked at the 'Raw' output but only at the 'Pretty' output. The latter shows that literal 1.
One thing obviously from looking at this PR for a few minutes btw. Is that we can't have an ID of 0 for all the items. JSON API spec states
Within a given API, each resource object’s type and id pair MUST identify a single, unique resource.Probably for what we are doing here that means the id for a media item should be the same as the path attribute (and potentially remove the path attribute).
This is a bit confusing. Do you propose to replace the '0' with the actual file path. I think that definitely makes sense. What confuses me though is the part in brackets 'and potentially remove the path attribute'. Not sure what you mean by that.
calling a GET on a non existent path give me 500 shouldn't be 404 ?
| Labels |
Added:
?
Removed: ? |
||
| Category | Administration Language & Strings Front End Plugins | ⇒ | Administration Language & Strings Libraries Front End Plugins |
calling a GET on a non existent path give me 500 shouldn't be 404 ?
Yes it should and I fixed it.
confirmed, as soon as i'll have more free time, i'll continue testing
p.s
is there some plugin for adapter like dropbox or whatever ?
please share download link
I have tested this item
I ran the following calls:
- Get item exists
- Get item non-exists
- Get list exists
- Get list non-exists
- Delete item exists
- Delete item non-exists
- Patch item exists
- Patch item non-exists
All returned the correct status code and expected result. Only difference I spotted is that the GET on a non-existent item will return:
{"errors":[{"title":"File not found: \/banners\/osmbanner1.pngd","code":404}]}
All other requests will return:
{"errors":[{"title":"File not found: peren22.jpg"}]}
Notice there is no code field.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34314.
| Labels |
Added:
?
Removed: ? |
||
All returned the correct status code and expected result. Only difference I spotted is that the GET on a non-existent item will return:
{"errors":[{"title":"File not found: \/banners\/osmbanner1.pngd","code":404}]}All other requests will return:
{"errors":[{"title":"File not found: peren22.jpg"}]}Notice there is no code field.
I missed while testing. Apologies for that.
The reason for this issue, is the somewhat sloppy implementation of web services exception handling. The way various exceptions are handled is not very consistent. In the current implementation it is also not easily possible to add custom exceptions in a clean manner. So I also had a hard time, trying to figure out which media manager exceptions to map to which web service exceptions.
In the current implementation of this media web service, I use the Joomla\CMS\MVC\Controller\Exception\Save exception, handled by the Joomla\CMS\Error\JsonApi\SaveExceptionHandler, to handle exceptions thrown by the media manager logic and caught as a result from POST and PATCH requests. The SaveExceptionHandler did not include the status code in responses. I remedied that in the latest update.
| Labels |
Added:
?
Removed: ? |
||
Thanx to @Quy and @YatharthVyas for finding all those nasty mistakes.
| Labels |
Added:
?
Removed: ? |
||
is there some plugin for adapter like dropbox or whatever ?
please share download link
Like I mentioned, information can be found here: #33724. The problem is, that Dropbox adapter doesn't work out of the box. So let's not postpone acceptance of this PR as long as there aren't any usable other adapters.
fine for me..
adapters and exception can be made in another pr
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34314.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34314.
can you take a look at #34634 @pjdevries please
can you take a look at #34634 @pjdevries please
Sorry to let you down, but I'm currently occupied with two important projects that take most of my time.
@bembelimen, @Quy, @wilsonge I am disappointed. Are we really going to release J4 without a web service implementation, which at least allows creation of articles with images? This PR does not affect or interfere with any existing code. What's wrong with it so it can not be included in the 4.0 release?
@pjdevries Good day. The plg_webservices_content.ini file has not been removed in the PR. But for some reason the file plg_webservices_content.sis.ini was deleted.
| Title |
|
||||||
If this one gets merged, then I can have a look if it would be possible to actually change the vue client to the official REST API and then we can deprecate the existing JSON controller.
What would also be nice is to have an endpoint to get a list of adapters. Like that you can fetch everything by API as you don't have to know the existing adapters yet.
@pjdevries could you please look into the feedback?
@pjdevries could you please look into the feedback?
A couple of months ago, when I initiated this PR, I had some spare time on my hands. Unfortunately it was not accepted at that time, event though it was tested and approved. Even unfortunately, I'm not in the same comfortable position as I was back then and can not spend time on it now.
@bembelimen what actually holds you back to merge this one as it is RTC? The issues from Phil can be made in a followup pr and my suggestions I can also do in another pr. Is there something important missing (was not reading every word in all the comments)?
@bembelimen what actually holds you back to merge this one as it is RTC? The issues from Phil can be made in a followup pr and my suggestions I can also do in another pr. Is there something important missing (was not reading every word in all the comments)?
The experience of the last 15 years, that if we don't fix it now, it will probably never be fixed. I like this PR and when ready I will merge it for sure.
I also think that all the comments can be fixed in next pr's, so i invite you to re-think your decision
There is a wrong ACL check in this PR that also needs addressing!
nothing that cannot be easy fixed in a next pr..... this pr is 4.1 based ... and already well known contributors expressed the intention to do it
So you are happy introducing security issues into Joomla... got it. This is why Joomla's code quality will never improve.
However it looks like @pjdevries is not around to make changes to his PR, but this is why GitHub allows maintainers to make changes to PR's before merging.
So you are happy introducing security issues into Joomla... got it. This is why Joomla's code quality will never improve.
this is what you got ? then NO i've been not able to explain myself, i'll retry...
this pr is for 4.1 so there is reasonable time to fix all the "comments".... hope it sounds a little better now
isn't it ?
This PR doesn't meant the minimum code standards and therefore should not be merged. Period. Fix those BEFORE merging not 15 years later if ever. That's how the Joomla code managed to get into such a crap state that it is now. Aim higher and stop trying to drag the code base down to your own standards.
"Comments" ARE CODE!
@bembelimen what actually holds you back to merge this one as it is RTC? The issues from Phil can be made in a followup pr and my suggestions I can also do in another pr. Is there something important missing (was not reading every word in all the comments)?
The experience of the last 15 years, that if we don't fix it now, it will probably never be fixed. I like this PR and when ready I will merge it for sure.
That is why you dont merge code that is knowingly broken
It's not even about comments - there are code typos, use of already deprecated PHP methods in both Joomla and in third party method calls, the license blocks are not formatted correctly, security checks are not done correctly, new methods are introduced with no documentation, since tags, or parameter references, if this is really the standard that Joomla is aiming for in 4.1 then we have already lost
i don't want to loose the effort put in this PR, for let me say "Code Styles"....
i want encourage people to do pr's, other's stuff can be fixed later.... and already some of us expicitley offered their help on this
Code style IS CODE.
And the objections are much more than code style with at least 2 security comments and 2 deprecated use of methods...
and already some of us expicitley offered their help on this
Then there is nothing stopping you placing a PR against the PR branch NOW - so it can be merged here before this is merged to Joomla
That's the way it's meant to happen!
| Status | Ready to Commit | ⇒ | Pending |
Update labels 2
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34314.
@pjdevries can you please check pjdevries#2
I'm very sorry, but like I mentioned before, I'm able to spend any time on this at the moment.
As @pjdevries has no time to finish it, would you @alikon take over and create a new PR with this code + your fixes to Joomla! 4.1-dev branch?
Then we can close this one and finish it in the new one, as I would really like to have this in 4.1.
I can also take it over, as I can spend some time on it in the next week.
Created a new pr #35788 which fixed most of the issue from @PhilETaylor. System tests are not yet finished and it also needs some more stabilization. I'm on holiday next week, so will not do much more in that time.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-10-08 12:05:41 |
| Closed_By | ⇒ | alikon | |
| Labels |
Added:
Language Change
?
?
?
Removed: ? ? ? |
||
maybe i'm doing something wrong