api5 issues

[lachlan-00]

issues i find while testing out the new api5 branch

[lachlan-00]

calling a function that doesn't exist still tries to call it

2020-10-13T08:41:34+10:00 [ampache] (log.lib) -> [Runtime Error] call_user_func() expects parameter 1 to be a valid callback, first array member is not a valid class name or object in file /var/www/music/server/json.server.php(86)

[lachlan-00]

advanced_search will just return everything even with a bad search

[lachlan-00]

bad data from catalog_action

Bad Request: clean{
    "error": {
        "errorCode": "4710",
        "errorAction": "catalog_action",
        "errorType": "task",
        "errorMessage": 18
    }
}

[lachlan-00]

2020-10-13T10:02:07+10:00 [ampache] (api.class) -> Handshake Attempt, IP:xxx.xxx.xxx.250 User: Version:5.0.0 
2020-10-13T10:02:07+10:00 [ampache] (api.class) -> Login Failed: Version too old

this needs to return an actual error too

[lachlan-00]

check that calling methods which require filters will cause errors, there are a lot of things that just return random or all the songs.

[lachlan-00]

look at total_count to include a DB cached total_count per object https://github.com/4phun/amproid/issues/6#issuecomment-711625990

[lachlan-00]

Okay, this is almost complete. @AshotN you're going to hate this because i've put all responses into objects which means your client will need to update for this version of the api (This has all been documented in the docs folder.)

I just want to finish off documentation first and then this will merge probably tomorrow.

This is going to be the first breaking change we make before moving onto source-changes so even though it's not really breaking a lot it's still a massive change in output data, especially for json.

[usox]

Looking forward having this in source-changes. After merging this, we will be able to start to make the api methods unit-testable :tada:

[lachlan-00]

it's in!

[usox]

Great! I'm going to (try to) merge this into source-changes at the weekend.

[AshotN]

Okay, this is almost complete. @AshotN you're going to hate this because i've put all responses into objects which means your client will need to update for this version of the api (This has all been documented in the docs folder.)

I just want to finish off documentation first and then this will merge probably tomorrow.

This is going to be the first breaking change we make before moving onto source-changes so even though it's not really breaking a lot it's still a massive change in output data, especially for json.

Sorry I have not been keeping up a lot with changes, school started and that plus work have stripped of the time I need to invest into Ampache. What do you mean that responses are in objects now?

[lachlan-00]

instead of returning an array, each item is returned in an object. i've documented each method on the api site but they all return something instead of an array.

http://ampache.org/api/api-json-methods

e.g. albums return album, playlists return playlist

{
    "album": [
        {
            "id": "81350",
            "name": "-2-",
            "artist": {
                "id": "292",
                "name": "Vinterriket"
            },
            "year": 2005,
            "tracks": "8",
            "disk": 1,
            "genre": [
                {
                    "id": "46",
                    "name": "Ambient"
                }
            ],
            "art": "https:\/\/music.com.au\/image.php?object_id=81350&object_type=album&auth=cfj3f237d563f479f5223k23189dbb34",
            "flag": 0,
            "preciserating": null,
            "rating": null,
            "averagerating": null,
            "mbid": "8844cfdc-4957-467b-ad70-d46e640d473e"
        }
    ]
}

Old responses were randomly in objects or not

[
    {
        "id": "57887",
        "name": "(It (Is) It) Critical Band",
        "artist": {
            "id": "9205",
            "name": "90 Day Men \/ GoGoGoAirheart"
        },
        "year": 2000,
        "tracks": 8,
        "disk": 1,
        "tag": [
            {
                "id": "674",
                "name": "Math Rock"
            },
            {
                "id": "1415",
                "name": "Post Rock"
            }
        ],
        "art": "https:\/\/music.com.au\/image.php?object_id=57887&object_type=album&auth=eeb9f1b6056246a7d563f479f518bb34",
        "flag": 0,
        "preciserating": null,
        "rating": null,
        "averagerating": null,
        "mbid": "d6fc36cc-9351-4b53-b7db-7f5b30804800"
    }
]

[AshotN]

Why is the album an array if there is only one ever returned?

[AshotN]

I know I am a little late to the party, but I think some of these object encapsulations don't make much sense.

I understand that the old API did return random objects inside arrays inside objects for no good reason. But the new API does something similar

For example: https://raw.githubusercontent.com/ampache/python3-ampache/master/docs/json-responses/stats%20(album).json

The stats album return, an object with a single property "album", but album is an array, so it shouldn't be singular. Why not just return the contents of the array without the object wrapper.

[{
        "id": "96351",
        "name": "Covers the Classics",
        "artist": {
            "id": "8980",
            "name": "Creedence Clearwater Revival"
        },
        "year": 2009,
        "tracks": "12",
        "disk": 1,
        "genre": [],
        "art": "https:\/\/music.com.au\/image.php?object_id=96351&object_type=album&auth=cfj3f237d563f479f5223k23189dbb34",
        "flag": 0,
        "preciserating": null,
        "rating": null,
        "averagerating": null,
        "mbid": "d823ef2f-0d91-4c65-a0ee-4377847d720d"
    },
    {
        "id": "89206",
        "name": "'74 Jailbreak",
        "artist": {
            "id": "869",
            "name": "AC\/DC"
        },
        "year": 2003,
        "tracks": "5",
        "disk": 1,
        "genre": [],
        "art": "https:\/\/music.com.au\/image.php?object_id=89206&object_type=album&auth=cfj3f237d563f479f5223k23189dbb34",
        "flag": 0,
        "preciserating": null,
        "rating": null,
        "averagerating": null,
        "mbid": "a2b73eb8-fe55-3bb6-a61e-b89f36d4b045"
    }
]

A bit more egregious of an example is when you request a single album for example. https://raw.githubusercontent.com/ampache/python3-ampache/master/docs/json-responses/album.json

The server returns an object with a single property album, which is an array, of a single object that contains the album information.

This is how I end up accessing that data through JavaScript return JSONData.album[0] as Album;

Why not simplify this to

{
    "id": "98156",
    "name": "-+",
    "artist": {
        "id": "1081",
        "name": "Mr.Kitty"
    },
    "time": 1552,
    "year": 2020,
    "tracks": "6",
    "disk": 1,
    "genre": [],
    "art": "https:\/\/music.com.au\/image.php?object_id=98156&object_type=album&auth=cfj3f237d563f479f5223k23189dbb34",
    "flag": 0,
    "preciserating": null,
    "rating": null,
    "averagerating": null,
    "mbid": "984aeded-cb31-495f-8a19-b38db868d4d6"
}

[AshotN]

Another concern I have, not sure if it warrants a separate issue.

The API still returns a HTTP code 200 for errors. As far as if this is permitted, is arguable. But if we are intending to return 200 for any API call, regardless if the request was successful or not, we should clearly document that.

[lachlan-00]

We'll have to go through it all again. This reasoning of arraying everything was to make the API always return the same thing.

[AshotN]

I can understand the appeal for consistency. But it definitely makes accessing the data require more code.

[lachlan-00]

That's true, the extra steps were semi intended but really only done as a "should be fine".

I've documented the whole thing so it should be easy enough to pick apart. Some things might be better to not return objects like you said with single items.

I'll start writing it up again on Monday.

[lachlan-00]

I'll start this more intently tomorrow but what about something more like this.

Functions that return more than one type of object (for example get_indexes) must return an object.

"song": {}|"album": {}|"artist": {}|"playlist": {}|"podcast": {}

Single type functions do not need to do this. but then what is their return data?

Instead of returning errors for missing/empty results i was returning an empty object array "album": {}

Artist should either return an artist or error. Artists can be 0,1,100+ so does it always return an array?

[AshotN]

For functions that are for multiple objects they should be plural. album -> albums etc

For functions that are for one, like getting a single album,

my earlier proposal was just a nameless object, and if nothing is found you can return An error like “invalid ID” or return a blank object. Your choice, but then all methods should be consistent with these rules.

[lachlan-00]

If it's an object array should the english matter? i've never really thought much about whether the plural is needed as you'd just for each that array or take the first result if that's what you wanted?

[usox]

Functions that return more than one type of object (for example get_indexes) must return an object.

"song": {}|"album": {}|"artist": {}|"playlist": {}|"podcast": {}

Can you explain me, why methods with multiple results need to be wrapped in an object? I don't get it. If this is really necessary, why don't you use a more generic approach? Just {"items": [...]}?

Single type functions do not need to do this. but then what is their return data?

My approach would be like @AshotN said.

Methods with single result (lookup, getById, ...):

• Return object {"id": "blah", ...}
• Error if entity was not found

Methods with multiple results (searches, batch loading, ...):

• Return list of objects [{"id": "blah", ...}]
• Return empty list if no objects were found

Another concern I have, not sure if it warrants a separate issue.

The API still returns a HTTP code 200 for errors. As far as if this is permitted, is arguable. But if we are intending to return 200 for any API call, regardless if the request was successful or not, we should clearly document that.

This has been discussed earlier. Imho this api should be seen as a rpc-like api, so application-level errors should not influence the http status code. This should always be 2xx, except errors on transport/protocol level occur. If the api should act rest-like, it should also be structured like a rest api - we should not mix things up here.

[lachlan-00]

On my phone now but the first half is a good summary. I'll start working on the docs/code to make this a bit clearer (instead of my vague rambles)

The 200 is documented for all method groups and only binary methods return different codes. I don't really see that needing to change back to 200 as you're either getting the data or you aren't

[usox]

The 200 is documented for all method groups and only binary methods return different codes. I don't really see that needing to change back to 200 as you're either getting the data or you aren't

I'm ok with that. It may be a good idea to make those methods easy recognizable, though. Examplewise by a naming pattern or namespaced methods names (binary.stream, binary.download, ...)

[AshotN]

If it's an object array should the english matter? i've never really thought much about whether the plural is needed as you'd just for each that array or take the first result if that's what you wanted?

Well the plural conveys that this is in fact an array and it makes more sense when programming with the API as well.

This has been discussed earlier. Imho this api should be seen as a rpc-like api, so application-level errors should not influence the http status code. This should always be 2xx, except errors on transport/protocol level occur. If the api should act rest-like, it should also be structured like a rest api - we should not mix things up here.

That's fine if we are returning the error as a 200, as long as we maintain the error format and are explicit in our documentation.

[lachlan-00]

okay, i'm busy for a few days but i'll be working on this for the next couple of days.

[AshotN]

One more thing to note,

Methods such as artists that have a include option. If include is not provided, it returns the count of albums and songs. If include is provided, it instead returns an array of the albums or songs.

I think this should be separated. The reason is that in JavaScript for example, or TypeScript rather, I have to define the model for an artist like so.

export type Artist = {
    id: number;
    name: string;
    tags: [];
    albums: Album[] | number; //The API returns the count if the include is set to false.
    songs: Song[] | number;
    art: string;
    flag: boolean;
    preciserating: number;
    rating: number;
    averagerating: number;
    mbid: string;
    summary: string;
    yearformed: number;
    placeformed: string;
};

Since albums is either an array or just a number, I have to check later what type it is again later cause there is ambiguity.

artist.albums.map((theAlbum) => { TypeScript throws a error here because it is unsure if albums is a number of an array.

I believe this should be seperated into albumCount, and a albums field. The albumCount can always be populated, and albums can be an empty array if nothing is included.

[lachlan-00]

we probably have a few duplicate actions in methods. that's actually why i put the counts in handshake/ping as well

[lachlan-00]

i'm goign to take the counts out of $include methods, it's a but useless and more harm when you can just ping/handshake to get the counts

** put them as count items as they're not totals and this matches subsonic.

[lachlan-00]

@AshotN next time you want to have a look over the changes, let me know what you're thinking.

So far i've just reverted back to whatever was in 4.x.x for single objects and will update docs/etc after i finish everything.

i just realised what was annoying me beforehand which was my initial reason for putting everything into objects.

arrayed single objects like playlist!

[
    {
        "id": "2200",
        "name": "rename",
        "owner": "user",
        "items": 0,
        "type": "private"
    }
]

instead of being able to say playlist['id'] you need to remember that it's an array and call playlist[0]['id']

now that it's finally clicked in my head i'll be able to fix up the ones that are weird. there are a few but not many at least. :)

[lachlan-00]

okay, org/api docs updated, methods and data examples updated. i'm out for the day but it's pretty ready now for you to look at again