Fix bytes concatination error in Client.search() #94

JOJ0 · 2022-04-17T07:50:37Z

Fixes TypeError: sequence item 0: expected str instance, bytes found #80
First steps of fixing the issue.

Todo:

Find out whether query is always tuple. -> Yes, since *args ensures that.
Write/fix tests.

Decide "join method" according to the type of "query".

A version of test_search, passing a bytes string to Client.search().

if anything else than tuple is passed.

Since *args is passed we'll always get a tuple which needs to be looked into for bytes first.

AnssiAhola · 2022-05-08T15:47:50Z

So I did some testing and came up with this

if query:
     items = []
     for item in query:
         if type(item) == bytes:
             items.append(item.decode())
         else:
             items.append(item)
     fields['q'] = ' '.join(items)
     ...

Edit: More pythonic way

if query:
    items = [item.decode() if type(item) == bytes else item for item in query]
    fields['q'] = ' '.join(items)

This allows for mixed types in *query

client.search("trash", b"80")
client.search("trash", "80")
client.search("trash"  "80")
client.search(b"trash80")
client.search("trash80")

All are valid.

Also to pass the tests with multiple queries
Add a res file in tests/res/database named search_q=trash%2080&per_page=50&page=1.json

I'm sure you can come up with better variable names etc. 😄
could maybe also be more pythonic?

This also needs to be tested with the real API but i'm sure it works fine
I tested few queries resulted from these changes in https://www.discogs.com/search/?q=<query>
and those worked fine

if necessary.

unicode and mixed combinations are accepted. Add a new fixture in tests/res/database responding to the multivalue search.

Add tests and corresponding fixtures to proof that passing keyword arguments (**fields) is working.

JOJ0 · 2022-05-17T08:33:06Z

Hi @AnssiAhola, sorry for not getting back to this earlier...other stuff....

Thanks a lot, that's a great solution!

Edit: More pythonic way

if query:
    items = [item.decode() if type(item) == bytes else item for item in query]
    fields['q'] = ' '.join(items)

This allows for mixed types in *query

Exactly what we should aim for here. Nice! I slightly reformatted the list comprehension for readability/linelength, have a look. What do you think?

I'm sure you can come up with better variable names etc. smile

Funnily enough: Not this time! No better ideas than "item" and "items" ;-) And it fits within 79 chars, which I like (even though we don't currently enforce that style in the project). Maybe I do have an idea: "part" and "parts"? Better?

This also needs to be tested with the real API but i'm sure it works fine I tested few queries resulted from these changes in https://www.discogs.com/search/?q=<query> and those worked fine

I'll see if I can do some real tests and also ask users having the problem with Discogs searches in beets if they want to try it.

I also added some tests to check if passing keyword arguments is actually working. We didn't have that yet, right? It's quite a pile of new tests now, please tell me which you think are not necessary or could be combined!

Am I correct that we now support mixed types when passing *query but not when passing **fields. Should we support that too?

JOJ0 · 2022-05-17T08:36:02Z

discogs_client/client.py

+            raise TypeError("{} method is expecting tuple, got {}.".format(
+                __name__, type(query)
+            ))
+


Do you agree this check is redundant and should be kicked out? As far as I understand *args always passes tuple.

Yeah, I think this can be left out.

AnssiAhola · 2022-05-17T09:39:09Z

Exactly what we should aim for here. Nice! I slightly reformatted the list comprehension for readability/linelength, have a look. What do you think?

Looks good to me!

Maybe I do have an idea: "part" and "parts"? Better?

Yeah. I like part/parts better. I think those fit better in this context.

I also added some tests to check if passing keyword arguments is actually working. We didn't have that yet, right? It's quite a pile of new tests now, please tell me which you think are not necessary or could be combined!

Didn't see any at least. And can't have too many tests imo! 😄

Am I correct that we now support mixed types when passing *query but not when passing **fields. Should we support that too?

It seems that **fields are passed straight to update_qs which casts all values to string, which might cause some issues if bytes are passed, but I we should look into that if/when it becomes an issue or at least make an separate PR for it.
Although docs for str() says that anything can be passed into it 🤔
What do you think?

discogs_client/discogs_client/utils.py

Lines 16 to 21 in 65e8e95

    
           def update_qs(url, params): 
        
               """A not-very-intelligent function to glom parameters onto a query string.""" 
        
               joined_qs = '&'.join('='.join((str(k), quote(str(v)))) 
        
                                    for k, v in params.items()) 
        
               separator = '&' if '?' in url else '?' 
        
               return url + separator + joined_qs

AnssiAhola · 2022-05-17T09:49:55Z

discogs_client/client.py

        if query:
-            fields['q'] = ' '.join(query)
+            items = [
+                item.decode() if type(item) == bytes else item for item in query


maybe this can be simplified with str() ?

items = [str(item) for item in query]

Haven't tested if that would work but given the docs say bytes can be passed into it, i don't see why not

I tried that when I initially started to work on this issue. str() doesn't change the string type, bytes stay bytes and unicode stays unicode, thus it doesn't avoid different types concat errors. Not 100% sure anymore but I'll doublecheck later today or tomorrow.

Hmmm I did not recall entirely correct: It seems the join does work but its not usable on another end:

items = [ str(item) for item in query ] for item in items: print(item) fields['q'] = ' '.join(items) print(fields)

$ python -m unittest discogs_client.tests.test_models.ModelsTestCase.test_multiterm_mixed_search trash b'80' {'q': "trash b'80'"} E ====================================================================== ERROR: test_multiterm_mixed_search (discogs_client.tests.test_models.ModelsTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/jojo/git/discogs_client/discogs_client/tests/test_models.py", line 64, in test_multiterm_mixed_search self.assertEqual(len(results), 13) File "/home/jojo/git/discogs_client/discogs_client/models.py", line 363, in __len__ return self.count File "/home/jojo/git/discogs_client/discogs_client/models.py", line 334, in count self._load_pagination_info() File "/home/jojo/git/discogs_client/discogs_client/models.py", line 289, in _load_pagination_info data = self.client._get(self._url_for_page(1)) File "/home/jojo/git/discogs_client/discogs_client/client.py", line 113, in _get return self._request('GET', url) File "/home/jojo/git/discogs_client/discogs_client/client.py", line 110, in _request raise HTTPError(body['message'], status_code) discogs_client.exceptions.HTTPError: 404: Resource not found. ---------------------------------------------------------------------- Ran 1 test in 0.001s FAILED (errors=1)

But with the decode solution:

items = [ item.decode() if type(item) == bytes else item for item in query ] for item in items: print(item) fields['q'] = ' '.join(items) print(fields)

$ python -m unittest discogs_client.tests.test_models.ModelsTestCase.test_multiterm_mixed_search trash 80 {'q': 'trash 80'} . ---------------------------------------------------------------------- Ran 1 test in 0.001s OK

JOJ0 · 2022-05-19T05:43:09Z

I also added some tests to check if passing keyword arguments is actually working. We didn't have that yet, right? It's quite a pile of new tests now, please tell me which you think are not necessary or could be combined!

Didn't see any at least. And can't have too many tests imo! smile

:-)

Am I correct that we now support mixed types when passing *query but not when passing **fields. Should we support that too?

It seems that **fields are passed straight to update_qs which casts all values to string, which might cause some issues if bytes are passed, but I we should look into that if/when it becomes an issue or at least make an separate PR for it. Although docs for str() says that anything can be passed into it thinking What do you think?

discogs_client/discogs_client/utils.py

Lines 16 to 21 in 65e8e95

def update_qs(url, params):

"""A not-very-intelligent function to glom parameters onto a query string."""

joined_qs = '&'.join('='.join((str(k), quote(str(v))))

for k, v in params.items())

separator = '&' if '?' in url else '?'

return url + separator + joined_qs

Wow! Now that's one hell of an ugly unreadable function! ;-)
No, seriously: It's not too complicated thus not too ugly to read actually but I would reformat it probably once I get the chance to ;-) And on the topic: Agreed that we shouldn't touch it without a cause. The same applies here: str() won't cast different string types and we are getting mixed types in one string. Not good actually.

Played around a little with the test of this method but I think what I did is a very unlikely case (different types for key and value)

+        base_url = 'http://example.com'
+        unicode_keyval = {'foo': 'bar'}
+        self.assertEqual(u(base_url, unicode_keyval), 'http://example.com?foo=bar')
+        bytes_keyval = 'http://example.com', {b'foo': b'bar'}
+        self.assertEqual(u(base_url, bytes_keyval), 'http://example.com?foo=bar')
+        mixed_keyval = 'http://example.com', {'foo': b'bar'}
+        self.assertEqual(u(base_url, mixed_keyval), 'http://example.com?foo=bar')

the bytes_keyval test:

======================================================================
FAIL: test_update_qs (discogs_client.tests.test_utils.UtilsTestCase)
update_qs helper works as intended
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jojo/git/discogs_client/discogs_client/tests/test_utils.py", line 22, in test_update_qs
    self.assertEqual(u(base_url, bytes_keyval), 'http://example.com?foo=bar')
AssertionError: "http://example.com?b'foo'=b%27bar%27" != 'http://example.com?foo=bar'
- http://example.com?b'foo'=b%27bar%27
?                    --   - ----   ---
+ http://example.com?foo=bar

the mixed_keyval test:

======================================================================
FAIL: test_update_qs (discogs_client.tests.test_utils.UtilsTestCase)
update_qs helper works as intended
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jojo/git/discogs_client/discogs_client/tests/test_utils.py", line 24, in test_update_qs
    self.assertEqual(u(base_url, mixed_keyval), 'http://example.com?foo=bar')
AssertionError: 'http://example.com?foo=b%27bar%27' != 'http://example.com?foo=bar'
- http://example.com?foo=b%27bar%27
?                         ----  ---
+ http://example.com?foo=bar


----------------------------------------------------------------------
Ran 7 tests in 0.007s

FAILED (failures=1)

Not sure if this is likely to ever happen, just playing around:

+        multi_mixed_keyval = {'foo': 'bar', b'faz': b'baz'}
+        self.assertEqual(u(base_url, multi_mixed_keyval), 'http://example.com?foo=bar&faz=baz')

======================================================================
FAIL: test_update_qs (discogs_client.tests.test_utils.UtilsTestCase)
update_qs helper works as intended
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jojo/git/discogs_client/discogs_client/tests/test_utils.py", line 20, in test_update_qs
    self.assertEqual(u(base_url, multi_mixed_keyval), 'http://example.com?foo=bar&faz=baz')
AssertionError: "http://example.com?foo=bar&b'faz'=b%27baz%27" != 'http://example.com?foo=bar&faz=baz'
- http://example.com?foo=bar&b'faz'=b%27baz%27
?                            --   - ---

I'll leave all this for now and let's move on with this PR's actual issue :-)

This reverts commit 6f68311. Not required at all since *args ensures a tuple.

AnssiAhola

LGTM! 👍

JOJ0 · 2022-05-19T13:00:15Z

Thanks for all the help and review @AnssiAhola 💯 ❤️

JOJ0 force-pushed the fix_search_type_err branch from cfcd059 to a63ebcb Compare April 17, 2022 07:55

JOJ0 mentioned this pull request Apr 17, 2022

Update client.py #79

Closed

JOJ0 force-pushed the fix_search_type_err branch from a63ebcb to 9098126 Compare April 17, 2022 08:05

JOJ0 added 2 commits May 5, 2022 07:22

Fix bytes concatination error in Client.search()

abac0dd

Decide "join method" according to the type of "query".

Add test_bytes_search

799c184

A version of test_search, passing a bytes string to Client.search().

JOJ0 force-pushed the fix_search_type_err branch from f2143fb to 916b371 Compare May 5, 2022 05:29

Add an early exit to Client.search

6f68311

if anything else than tuple is passed.

JOJ0 force-pushed the fix_search_type_err branch from 916b371 to 6f68311 Compare May 5, 2022 05:29

Fix bytes detection in Client.search

e8f1777

Since *args is passed we'll always get a tuple which needs to be looked into for bytes first.

JOJ0 added 3 commits May 17, 2022 07:49

Client.search: Decode bytes before concatination

19c32df

if necessary.

Client.search: Add more tests proofing that bytes

05a87fb

unicode and mixed combinations are accepted. Add a new fixture in tests/res/database responding to the multivalue search.

Client.search: Add tests checking kwargs search

1664225

Add tests and corresponding fixtures to proof that passing keyword arguments (**fields) is working.

JOJ0 force-pushed the fix_search_type_err branch from 08dc4cd to 1664225 Compare May 17, 2022 08:26

JOJ0 commented May 17, 2022

View reviewed changes

JOJ0 marked this pull request as ready for review May 17, 2022 09:30

AnssiAhola reviewed May 17, 2022

View reviewed changes

Revert "Add an early exit to Client.search"

037dd3f

This reverts commit 6f68311. Not required at all since *args ensures a tuple.

AnssiAhola approved these changes May 19, 2022

View reviewed changes

JOJ0 merged commit 6209dc3 into master May 19, 2022

JOJ0 deleted the fix_search_type_err branch May 19, 2022 13:02

JOJ0 mentioned this pull request May 19, 2022

TypeError: sequence item 0: expected str instance, bytes found #80

Closed

JOJ0 mentioned this pull request Mar 10, 2023

discogs_client version check beetbox/beets#4698

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bytes concatination error in Client.search() #94

Fix bytes concatination error in Client.search() #94

JOJ0 commented Apr 17, 2022 •

edited

Loading

AnssiAhola commented May 8, 2022 •

edited

Loading

JOJ0 commented May 17, 2022 •

edited

Loading

JOJ0 May 17, 2022 •

edited

Loading

AnssiAhola May 17, 2022

AnssiAhola commented May 17, 2022

AnssiAhola May 17, 2022

JOJ0 May 18, 2022 •

edited

Loading

JOJ0 May 19, 2022 •

edited

Loading

JOJ0 commented May 19, 2022 •

edited

Loading

AnssiAhola left a comment

JOJ0 commented May 19, 2022

Fix bytes concatination error in Client.search() #94

Fix bytes concatination error in Client.search() #94

Conversation

JOJ0 commented Apr 17, 2022 • edited Loading

AnssiAhola commented May 8, 2022 • edited Loading

JOJ0 commented May 17, 2022 • edited Loading

JOJ0 May 17, 2022 • edited Loading

Choose a reason for hiding this comment

AnssiAhola May 17, 2022

Choose a reason for hiding this comment

AnssiAhola commented May 17, 2022

AnssiAhola May 17, 2022

Choose a reason for hiding this comment

JOJ0 May 18, 2022 • edited Loading

Choose a reason for hiding this comment

JOJ0 May 19, 2022 • edited Loading

Choose a reason for hiding this comment

JOJ0 commented May 19, 2022 • edited Loading

AnssiAhola left a comment

Choose a reason for hiding this comment

JOJ0 commented May 19, 2022

JOJ0 commented Apr 17, 2022 •

edited

Loading

AnssiAhola commented May 8, 2022 •

edited

Loading

JOJ0 commented May 17, 2022 •

edited

Loading

JOJ0 May 17, 2022 •

edited

Loading

JOJ0 May 18, 2022 •

edited

Loading

JOJ0 May 19, 2022 •

edited

Loading

JOJ0 commented May 19, 2022 •

edited

Loading