-
Notifications
You must be signed in to change notification settings - Fork 189
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
Audio Metadata #7490
Audio Metadata #7490
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
"album": "Audio.album", | ||
"albumArtist": "Audio.albumArtist", | ||
"artist": "Audio.artist", | ||
"title": "Audio.title", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
seems to have a specific meaning in kql, so e.g. audio.artist
cannot be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly are you trying to search for?
GET /drives/{drive-id}/root/search(q='Motörhead')
or GET /me/drive/search(q='Motörhead')
to include all drives the user has access to is a full text search that includes content.
I think GET /drives/{drive-id}/items?$filter=audio/artist eq Motörhead
better captures what you are0 trying to do ...
More info on $filter https://learn.microsoft.com/en-us/graph/filter-query-parameter?tabs=http
now therte also is a search endpoint in ms graph thet can be used to query ... everything:
POST https://graph.microsoft.com/beta/search/query
Content-Type: application/json
{
"requests": [
{
"entityTypes": [
"driveItem"
],
"query": {
"queryString": "Motörhead"
},
"from": 0,
"size": 25
}
]
}
The queryString
is a KQL query, but I cant test it on ms graph because AFAICT I don't have enough permissions to call that endpoint. Nevertheless, xamples for queries can be found in https://learn.microsoft.com/en-us/graph/search-concept-files#example-5-use-filters-in-search-queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I want to search for "artist:Motörhead" and get all songs that have Motörhead as artist and none that only have "(Motörhead Cover)" in the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes, that filter seems reasonable... is that already implemented in oCIS? Should that just work automatically or do I need to implement it somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it ❤️ How could we have lived without it?
Awesome proof that it can be done for specific meta data, although this patch is big.
There are multiple aspects why this looks big:
This PR could be split up, I just wanted to send it in total first, so we can discuss the big picture and don't add small incremental changes that lead nowhere. edit: |
I've pushed new commits with a more dynamic approach using reflection, which is probably slightly slower but more maintainable. It also makes it much easier to add more data types and is less error prone. A third approach could be to autogenerate I'm not insisting on any approach, do you have any thoughts? |
Can probably be simplified with https://pkg.go.dev/github.com/mitchellh/mapstructure which is used by reva anyway |
a3646bf
to
ec78fc5
Compare
b955f48
to
801566d
Compare
The state right now extracts audio metadata, writes them into the index and into the arbitrary metadata storage. What I've removed for now although it was part of the initial PR: This is ready for another round of review from my point of view. |
I don't think it's worth porting as that would likely mean more loop iterations because it doesn't exactly do what we need and our methods using reflection are reasonable enough... |
Changelog added |
Updated, I think I addressed everything from @kobergj 's review |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me.
* Add audio facet to search protobuf message * Add audio metadata to search index * Return audio facet from search if available * Store audio metadata in arbitrary metadata * Add audio facet to driveItems listings * Make tests coding style more consistent * Fix tests * Add changelog * Make valueToString code more defensive * Log status code as well
Thanks! |
CURRENT STATE
We now extract audio metadata, write it into the index and into the arbitrary metadata storage.
There are a bunch of tests for testing the marshalling and unmarshalling between the different data structures in play.
Audio Metadata is added to Graph Api listings as Audio Facet of DriveItems.
The search service responds with audio metadata, but neither Graph Api (as it has no search yet) nor WebDAV Api currently make it part of their search responses. I would like to tackle that in a followup.
INITIAL PR
Description
This PR is a draft for adding audio metadata to the search index and the metadata storage. It uses that metadata to implement the audio facet (added in owncloud/libre-graph-api#120).
Moreover the metadata is added to search results and WebDAV propfinds as custom
<oc:audio.*>
props.I had to partially update
libegraph-api-go
to make theAudio
model available and had to update reva to add the props to propfinds, I've done that in thevendor/
folder just to get started and will obviously upstream those changes once we agree we want to go down this road.On the one hand I've been surprised how easy it was to implement this in the end (kudos for having everything in place for me!), on the other hand I've found it quite cumbersome to do lots and lots of error-prone (de)serializing manually:
In search service:
libregraph.Audio
struct (okay, this makes total sense, this does some actual semantic mapping)libregraph.Audio
struct -> metadata (map[string]string
) for storagelibregraph.Audio
) -> protobufsearchMessage.Audio
Then on the consuming end:
libregraph.Audio
libregraph.Audio
to WebDAV propssearchMessage.Audio
-> WebDAV propssearchMessage.Audio
tolibregraph.Audio
(then we could reuse that when serializing to webdav)All of this is almost the same but not quite - any ideas how to handle this more automatically or at least have all logic in a more central place? Right now one has to touch a lot of places to add a new facet to items.
This could probably be handled almost automatically using reflection, I'm just not sure of the performance penalty this might bring.
Used prefixes etc are totally up for debate, this is basically a PoC - not knowing the oCIS codebase too well I'm happy about any constructive feedback.
Related Issue
Motivation and Context
This is a personal pet peeve of mine, there's been no company request to implement this and as such there's no priority to deal with this PR - I would just personally appreciate it ;-)
I want to query my personal music collection by metadata.
Screenshots (if appropriate):
Regular PROPFIND:
WebDAV search:
Graph API Listing:
Types of changes
Checklist: