search

maanlamp / OBA-wrapper

A wrapper for the OBA-API to intuitively request data.
MIT License
15 stars 2 forks source link

Can't use /availability endpoint #2

Closed jeroentvb closed 5 years ago

jeroentvb commented 5 years ago

I found a few things that prevent the /availability endpoint from working with the wrapper.

ID not working

An id for this endpoint would look like this |oba-catalogus|1201384, the problem however is that the | gets parsed to %7C which causes an error. https://github.com/maanlamp/OBA-wrapper/blob/4833006a37ccb366d9b98374b8be887ffa2c4fc8/js/index.js#L45-L48 Changing id in the code above to frabl works, because the frabl doesn't use special characters.

Unknown parameters

&pagesize=1&refine=false are given to any built url by default. But those parameters aren't known by the /availability endpoint, so it causes an error. https://github.com/maanlamp/OBA-wrapper/blob/4833006a37ccb366d9b98374b8be887ffa2c4fc8/js/index.js#L83-L85 I would suggest checking what endpoint is used and applying those parameters accordingly.

Sidenote: why are you setting refine=false on every normal /search? I thought refine=true returned extra info about the search results.

Supressed TypeError: Cannot read property '_text' of undefined

When I manually fixed (in a very dirty way, which breaks /search) the above issues I got a Supressed TypeError: Cannot read property '_text' of undefined https://github.com/maanlamp/OBA-wrapper/blob/4833006a37ccb366d9b98374b8be887ffa2c4fc8/js/modules/ping.js#L13-L17 I believe it fails because json.aquabrowser.meta.count doesn't exist in the response of the /availability endpoint.

maanlamp commented 5 years ago

@jeroentvb Thanks for the issue!

You said:

An id for this endpoint would look like this |oba-catalogus|1201384, the problem however is that the | gets parsed to %7C which causes an error.

This is because the pipe character (|) is not a safe url character. Everything that gets sent through the url (the id in this case), will be URLEncoded. I'm debating whether I'll allow it in the url, or just make everyone use the frabl :)

pagesize=1&refine=false are given to any built url by default. But those parameters aren't known by the /availability endpoint, so it causes an error.

This is interesting. Since I hadn't tested the availability endpoint, I didn't know this would happen. I will definitely fix this asap.

I would suggest checking what endpoint is used and applying those parameters accordingly.

Yep :)

Sidenote: why are you setting refine=false on every normal /search? I thought refine=true returned extra info about the search results.

That code is for a ping to the server, with which I'm looking at how many results there are to get, and to get the request context. Setting refine=true on a ping will slow it down, while a ping is supposed to be very minimal in payload and time footprint. The actual request has no set refine, so you can set that yourself.

I believe it fails because json.aquabrowser.meta.count doesn't exist in the response of the /availability endpoint.

Right, another side-effect of my lack of testing. Thanks for looking into it -- I'm fairly sure you are right.

jeroentvb commented 5 years ago

I'm debating whether I'll allow it in the url, or just make everyone use the frabl :)

I would make everybody use the frabl, since as you said, the pipe character isn't url safe and it's required to use the id as far as I know.

That code is for a ping to the server, with which I'm looking at how many results there are to get, and to get the request context. Setting refine=true on a ping will slow it down, while a ping is supposed to be very minimal in payload and time footprint. The actual request has no set refine, so you can set that yourself.

Aha, makes sense.

maanlamp commented 5 years ago

After fooling around in the code, the simplest solution I came up with is in https://github.com/maanlamp/OBA-wrapper/commit/f976c78d1d7186d0a3cbbb5152a6f0da4fc685ff.

Sorry you have to use a custom method that doesn't make use of the stream etc. This "just works™️". Maybe I'll revisit this later for a cleaner solution.