discogs / discogs_client

DEPRECATED - Official Python Client for the Discogs API
http://www.discogs.com/developers
Other
479 stars 132 forks source link

Release attributes return different values depending on previosly accessed fiels #32

Closed andriykohut closed 9 years ago

andriykohut commented 10 years ago
from discogs_client import Client, Release

client = Client('python')
releases = [i for i in client.search('Flying Horseman') if isinstance(i, Release)]

r = releases[3]
print r.title           # Flying Horseman - City Same City
print r.data['title']   # Flying Horseman - City Same City
print r.master.title    # City Same City
print r.title           # City Same City

This behavior is really strange: Accessing Release.master.title also changes Release.title. I can't find a way to get just album name without artist, it works only if I access master first.

rodneykeeling commented 10 years ago

Thanks for this report! This is definitely odd behavior. I will look into getting this fixed ASAP.

rodneykeeling commented 10 years ago

Okay, what's happening is that r is being set from the releases list, which is a list of search results. Search results are stubbed releases, so when you call a property that doesn't exist in the stubbed release, it looks like it fetches the entire release. This is why the name changes. r.title initially is the title property of the search result, but when you call .master on it, it has to actually get the entire release to get that information. Once it does, it updates its title.

So to circumvent this, instead of: r = releases[3] You can do: r = client.release(releases[3].id)

That way you have a real Release object initially instead of a stubbed result.

Hope this helps!

andriykohut commented 10 years ago

Thanks, seems to be working as expected. Is there a plan to fix search so it will return complete objects instead of search results?

This should do the trick:

diff --git a/discogs_client/models.py b/discogs_client/models.py
index 1adeb1a..d750ec5 100644
--- a/discogs_client/models.py
+++ b/discogs_client/models.py
@@ -420,6 +420,9 @@ class MixedPaginatedList(BasePaginatedResponse):
         if item['type'] in ('label', 'artist'):
             item['name'] = item['title']

+        elif item['type'] == 'release':
+            del item['title']
+
         return CLASS_MAP[item['type']](self.client, item)

But I'm not completely shure whether this won't cause any other problems. What do you think?

rodneykeeling commented 10 years ago

We don't plan on adding full objects to the search results, no, but I might look into having search results return just the title for the title field (instead of artist_name — title) so fields at least match up exactly. The reason we don't return full objects is that search is already an expensive aspect of an API (in terms of both computation and bandwidth), so it's actually better to have 2 requests instead of 1 bloated request in many cases. This isn't exclusive to Discogs, though; Github (and others) implements this same behavior with search: https://developer.github.com/v3/#summary-representations

andriykohut commented 10 years ago

Yeah, I totally get it. I was a little bit confused by this stub object, I thought it fetches some release data anyway.

By the way, this models like Release, Artist etc. are lazy and shouldn't fetch additional data if there is no calls to attributes, so it seems to me that this problem is more about Release's being created from search result which have too much data. When I try to access title first time it finds the first one from search result, accessing other attributes, namely ones missing from search result json triggers refresh method and updates every field.

This behavior is reasonable for all fields which are identical in stub release from search result and full release, unfortunately this doesn't apply for release title (and maybe smth else). I think that removing those from _dict in Release.__init__ should provide proper behavior.

Thanks a lot for broad response

rodneykeeling commented 9 years ago

Closing this for now.