oknozor / musicbrainz_rs

A wrapper around the musicbrainz API
MIT License
38 stars 18 forks source link

Query builder does not fit musicbrainz lucene query for certain entities #58

Closed oknozor closed 3 years ago

oknozor commented 3 years ago

The QueryBuilder derive macro we use for lucene query does not fit with some of the search queries :

For instance release-groups search accept the following fields :

While top level fields of the ReleaseGroup entity (on musicbrainz_rs side) only refers to artists via artist_credit.

We end up with a single query parameter artist_credit in the release group generated builder. We can rename it with the current implementation but we still lacks two artists related fields.

In my opinion the best approach would be to implement this manually and get rid of the query builder macro.

ritiek commented 3 years ago

I see. If this is the case, would it be a good idea to create a different struct for each entity made specifically for search purpose?

I mean ReleaseGroup has all the searchable fields on the api defined here. So we'd create a new struct containing all these searchable fields:

use lucene_query_builder::QueryBuilder;
use crate::entity::alias::Alias;
...

#[derive(Debug, Serialize, Deserialize, PartialEq, Clone, QueryBuilder, Default)]
struct ReleaseGroupSearch {
    alias: Alias,
    arid: String,
    artist: String,
    ...
}

and then we could make use of these structs when making searches:

use musicbrainz_rs::entity::release_group::ReleaseGroupSearch;

let query = ReleaseGroupSearch::query_builder()
    .arid("3377f3bb-60fc-4403-aea9-7e800612e060")
    .and()
    .artist("the chainsmokers")
    .build();

(here arid refers to Halsey)

which would result this query: query=arid:3377f3bb-60fc-4403-aea9-7e800612e060 AND artist:"the chainsmokers"

Does this sound like a good idea? Otherwise we can already write our own queries manually this way:

let query_param = "arid:3377f3bb-60fc-4403-aea9-7e800612e060 AND artist:\"the chainsmokers\"".to_string();
let query = format!("query={}", query_param);
oknozor commented 3 years ago

Implementing a separate search struct and still using the query builder derives seems to be a great compromise, let's go for it !

oknozor commented 3 years ago

implemented in #69