search

nextcloud / notes

✎ Distraction-free notes and writing
https://apps.nextcloud.com/apps/notes
GNU Affero General Public License v3.0
623 stars 135 forks source link

External API for attachments #826

Open korelstar opened 2 years ago

korelstar commented 2 years ago

785 introduced a new functionality for attachments (see also #74). The internal API contains two new straight-forward endpoints:

I would like to expose this functionality also to the external API so that third-party apps can use it. However, I'm not sure, if these endpoints are sufficient for you app developer @nextcloud/notes @stefan-niedermann. Therefore, please provide feedback!

What I have in mind is the following: when there are these two API changes only, your app will have to parse a note for image tags (e.g. ![label](relative image path)) and (optionally) links (e.g. [label](relative link path)) and then run subsequent requests to the GET attachment endpoint.

Another alternative is to do this parsing (also) on the server and provide a list of attachments together with the note's response (new attribute besides title, content etc). However, this will slow down synchronization and your app will still have to parse the note in order to show those images.

Hence, I vote for staying with these two changes (just add GET attachment and POST attachment) and the client will have to do the rest. Do you agree with this approach or do you see any other requirements for this API? @nextcloud/notes @stefan-niedermann

muety commented 2 years ago

I'd be happy to see this implemented! :tada:

stefan-niedermann commented 2 years ago

@korelstar sorry for the quite late reply - I did watch all changes of the Notes server repository and I am quite up to date. Please grant me some more time to review the concept of the attachment API, let's say one more week. I am unfortunately low on time (as always sigh 😮‍💨), but I am looking forward to implement the API in the Android app.

korelstar commented 2 years ago

@stefan-niedermann No problem, looking forward for your feedback :smiley:

stefan-niedermann commented 2 years ago

Alright, I had some time to read through the related issues and PRs. I know i am quite late to the party, but if possible, I'd like to discuss a few things (at least ask you to explain them 😉):

What I've understand so far

  1. In the markdown, the attachment paths are relative to the notes folder (the one which can also be changed via the settings API)? Example:
![Frog](../frog.jpg)

Given the notes folder is set to /Notes this means that the frog image is in the root directory, next to the /Notes folder, correct?

Clients are supposed to prefix the attachments api endpoint at rendering time in order to retrieve the actual file.

Open questions with this approach

  1. How do we deal with changing the notes folder? Does the notes server app update all notes? Will all attachments be broken?
  2. How do we deal with moving the attached files to other places?
  3. One key feature that I miss compared to the preview endpoint of Nextcloud files is to define a maximum size for x and y, so the response will get downscaled and cached as thumbnail before delivering it to the client. Would definitely appreciate if this could be adapted (given we stick with the attachments endpoints

Proposal for changes

Again, I am late to the party, so it's totally relatable to not make such deep changes anymore. Though I'd like to suggest and discuss some changes:

My first thought was to just use the fileId which does not change when moving stuff around. Querying content would also allow to access the preview endpoint.

Uploading a file

Just use the existing Nextcloud files API to upload the file. The fileId could be the link (maybe a second request needs to be done to retrieve the fileId):

![Frog](/f/123456)

Opening this as a plain link does work and will redirect you to the file in the Nextcloud files app (especially on Android, where users can be redirected to the Nextcloud Android app instead of just getting downloaded the attachment in a browser). This should also work nicely with other 3rd party apps like previewgenerator. Another advantage of this approach is that you wouldn't need to reinvent the wheel and add / maintain additional API endpoints rather than just use existing ones. The implementation could be completely possible on the client side (web interface in your case).

Downloading a file / Preview for rendering

You would need to rewrite the URL in the preview mode from

/f/123456 to /index.php/core/preview?fileId=123456 (optionally with x / y query params)

korelstar commented 2 years ago

Thanks for you feedback, @stefan-niedermann !

What I've understand so far

1. In the markdown, the attachment paths are relative to the notes folder (the one which can also be changed via the [settings API](https://github.com/nextcloud/notes/blob/master/docs/api/v1.md#settings))?

No, the attachment paths are relative to the note. E.g., if the notes path is /notes and the note's category is Animals, then the attachment path frog.jpg will be resolved to /notes/Animals/frog.jpg and the attachment path ../frog.jpg will be resolved to /notes/frog.jpg.

For security reasons, we've restricted the path resolution to the notes path, for now. E.g., if the notes path is /notes and the note's category is Animals, then the attachment path ../../frog.jpg is invalid.

Clients are supposed to prefix the attachments api endpoint at rendering time in order to retrieve the actual file.

No, clients do not need to worry about this. Clients just pass the notes ID and the relative attachment path to the API.

Open questions with this approach

  1. How do we deal with changing the notes folder? Does the notes server app update all notes? Will all attachments be broken?

No, since the path is relative to the notes path, it is fully transparent. But: Changing the note's category may lead to losing the connection to the attachment. This could be improved (see at the end of this comment).

  1. How do we deal with moving the attached files to other places?

Then the attachment can't be found anymore. But I think this is acceptable.

  1. One key feature that I miss compared to the preview endpoint of Nextcloud files is to define a maximum size for x and y, so the response will get downscaled and cached as thumbnail before delivering it to the client. Would definitely appreciate if this could be adapted (given we stick with the attachments endpoints

This works only for image attachments - which will be of course the most used case, but please note this circumstance.

Which size would be preferred? Should the client be able to define the size on its own?

Proposal for changes

Again, I am late to the party, so it's totally relatable to not make such deep changes anymore. Though I'd like to suggest and discuss some changes:

My first thought was to just use the fileId which does not change when moving stuff around. Querying content would also allow to access the preview endpoint.

I see two big disadvantages with the ID approach:

Uploading a file

Just use the existing Nextcloud files API to upload the file.

As far as I can see, this API uses the WebDAV protocol. Currently, the notes API is kept very simple so that implementing clients is very easy. This would not be the case anymore. Furthermore, the client needs to derive the absolute path of the attachment - with all side effects.

The fileId could be the link (maybe a second request needs to be done to retrieve the fileId):

![Frog](/f/123456)

Opening this as a plain link does work and will redirect you to the file in the Nextcloud files app (especially on Android, where users can be redirected to the Nextcloud Android app instead of just getting downloaded the attachment in a browser). This should also work nicely with other 3rd party apps like previewgenerator. Another advantage of this approach is that you wouldn't need to reinvent the wheel and add / maintain additional API endpoints rather than just use existing ones. The implementation could be completely possible on the client side (web interface in your case).

I see your point, but I'm still not happy with the disadvantages of using the file ID (see above).

Looking at the Text app

The Nextcloud Text app inserts the following Markdown, when adding an image: ![image.jpg](image.jpg?fileId=556092#mimetype=image%2Fjpeg&hasPreview=true)

The main part is the human readable file path, followed by the file ID (and some other metadata - do we need them, too?).

What about transferring this approach to Notes? The API could take both file path and file ID. If there is no file with given path, then it would search for the file ID. However, the notes app should update the path, if a file is moved.

The big advantage: Markdown files created with the Notes app are compatible to those created with the Text app (and the other way around). I really think, we should keep this compatibility (especially if we are able to do #652 at some time in the future).

What do you think of this "compromise"?

newhinton commented 2 years ago

@korelstar As you already said, we should under no circumstance use the file-id as the main reference for the image. That would completely break any markdownviewer out there, and there is no way to 'manually' edit the files. However, adding get-parameters to the image.jpg should be possible, but we need to check if it is compliant and works with clients. (I checked my desktop-client, and adding a query does not work.) We should be very careful with that.

In my opinion, we should not touch the filepath at all. If we need parameter, we should include them in the api endpoint, not in the filepath that is stored in the markdown.

GET /notes/{noteid}/attachment/{size}?path= or whatever we need.

The question is: do we even need anything besides the noteid and the path? How would the file ID or metadata be relevant, since we only have to render the image as per markdown?

Also we should be very careful what we import from the text-app, since they basically break most conventions that markdown should stick to. The Text-App should not be our role model.

stefan-niedermann commented 2 years ago

Alright, thanks for the clarification!

Which size would be preferred? Should the client be able to define the size on its own?

Yes, ideally just like the default files API by defining x and y query params, semantically meaning that the image should be downscaled to a size that fits into the dedined size ("contains"). Ignore the params if the file is not an image / can not be downscaled.

In my opinion, we should not touch the filepath at all. If we need parameter, we should include them in the api endpoint, not in the filepath that is stored in the markdown.

Just as additional query params like

GET /notes/{noteid}/attachment?path=frog.jpg&x=1920&y=1080

(Quite selfish, but the nextcloud-commons glide module can already handle this automatically by the screen size 😜)

I agree with you that the approach is good and we don't need to change anything to file IDs or so 👍.

newhinton commented 2 years ago

GET /notes/{noteid}/attachment?path=frog.jpg&x=1920&y=1080

It seems i had a moment of clouded judgement, this is no problem at all since the params could be handled by the client outside of markdown. I have to retract my objections, this would be just fine!

Does the glide module append the x&y params? Or how does it handle it? I have only implemented url-rewriting, so i didn't stumble across the actual handling of the image request. Though it did seem to work out of the box after a proper url was provided after parsing the markdown, including caching

newhinton commented 2 years ago

Is there anything that needs to be done to publish this? It seems to me that there are no open questions and all requirements are satisfied, so we should be able to finalize this and get that functionality into the apps

korelstar commented 2 years ago

Yes, there is not much to be done. However, I didn't find the time for this. If you want to help, I would be very happy. Open tasks are:

stefan-niedermann commented 2 years ago

It should be evaluated, if the download method could just redirect to the existing preview service of the Nextcloud files API

@korelstar did you already have some time to evaluate whether it is possible to forward x and y query arguments to the preview API? For a mobile client it is essential in my opinion to not transfer the full sized images. I'd love to implement support i the Android client, so I'm looking forward to the official API 😉

korelstar commented 2 years ago

@stefan-niedermann Sorry for being late on this. But yes, my plan is to re-use the existing preview API and provide sizing parameters in the Notes API. Hopefully, I will soon find some time to implement this.

theatischbein commented 1 year ago

The richtext editor already support this. Is it wanted to support the edit/preview mode parallel to the richtext editor ?

korelstar commented 1 year ago

This issue is about the external API which is used by e.g. the Android app. Therefore, also the rich text editor does not support this yet. Please see here for the steps to be done: https://github.com/nextcloud/notes/issues/826#issuecomment-1206832073 This would be the same for both types of editors.

theatischbein commented 1 year ago

Thanks for your answer! I am still a bit confused. The richtext editor uses the following endpoint: POST http://localhost:8080/apps/text/attachment/upload?documentId=570&sessionId=176&sessionToken=token&shareToken= for uploading attachements. This should also be usable/accessable for the Android app, when using the richtext editor , shouldn't it ?

I would like to give it try to implement the API endpoint, but still not get the idea to expose this feature to third party apps.

And I do get your answer right, that both editors should be supported ?

HammyHavoc commented 1 month ago

Is anyone still working on this? Being able to have voice notes synced via Nextcloud Notes in the likes of QuillPad would be amazing for capturing lyric and song ideas without hassle.

newhinton commented 1 month ago

The code has been done for a long while. All kinds of attachments should be supported (though all non-image-attachments would just allow you to download them, not auto-play them in case of audiofiles), once it gets merged

Edit:

Sorry, for the web-ui this is already supported. This is about allowing external apps to use this feature.