Merge lp:~gary-lasker/software-center/fix-lp920741 into lp:software-center/5.2

Proposed by Gary Lasker
Status: Merged
Merged at revision: 3052
Proposed branch: lp:~gary-lasker/software-center/fix-lp920741
Merge into: lp:software-center/5.2
Diff against target: 153 lines (+32/-9)
7 files modified
softwarecenter/backend/installbackend_impl/aptd.py (+2/-2)
softwarecenter/db/application.py (+2/-1)
softwarecenter/ui/gtk3/panes/availablepane.py (+4/-2)
softwarecenter/ui/gtk3/panes/installedpane.py (+2/-1)
softwarecenter/ui/gtk3/panes/softwarepane.py (+2/-1)
softwarecenter/ui/gtk3/views/appdetailsview.py (+3/-2)
test/test_aptd.py (+17/-0)
To merge this branch: bzr merge lp:~gary-lasker/software-center/fix-lp920741
Reviewer Review Type Date Requested Status
software-store-developers Pending
Review via email: mp+112161@code.launchpad.net

Commit message

* lp:~gary-lasker/software-center/fix-lp920741:
   - fix UnicodeDecodeError when a commercial app's title contains
     a unicode character (LP: #920741)

Description of the change

This branch fixes a UnicodeDecodeError when a commercial app's name has a non-ascii character in it, bug 920741. It's a very simple fix and actually only involves a change to a logger info statement.

To post a comment you must log in.
Revision history for this message
Michael Vogt (mvo) wrote :

On Tue, Jun 26, 2012 at 04:17:22PM -0000, Gary Lasker wrote:
> Gary Lasker has proposed merging lp:~gary-lasker/software-center/fix-lp920741 into lp:software-center/5.2.
>
> Requested reviews:
> software-store-developers (software-store-developers)
> Related bugs:
> Bug #920741 in software-center (Ubuntu): "UnicodeDecodeError in add_repo_add_key_and_install_app when a appname has special chars in it"
> https://bugs.launchpad.net/ubuntu/+source/software-center/+bug/920741
>
> For more details, see:
> https://code.launchpad.net/~gary-lasker/software-center/fix-lp920741/+merge/112161
>
> This branch fixes a UnicodeDecodeError when a commercial app's name has a non-ascii character in it, bug 920741. It's a very simple fix and actually only involves a change to a logger info statement.
[..]

Thanks for your work on this branch! The code change looks good. Could
you also please add a regression test to ensure we do not regress here
in the future, especially when we move from python2 to python3? Manual
instructions how to test this against a real app would also be good
(afaik there is one with that problem currently available?).

Plus we use the Application object in a (small) number of places in a
logger call so it would be good to know if it crashes everywhere or
just in this particular place and if it crashes in all logger contexts
we need to update that code too.

Cheers,
 Michael

Revision history for this message
Dave Morley (davmor2) wrote :
Download full text (3.9 KiB)

I've tried to run the install for elementals once more and got the following:

davmor2@boromir:~/tmp/fix-lp920741$ export SOFTWARE_CENTER_AGENT_INCLUDE_QA=1
davmor2@boromir:~/tmp/fix-lp920741$ ./software-center
2012-06-27 11:05:39,084 - softwarecenter - INFO - Using data (UI, xapian) from current dir
2012-06-27 11:05:40,548 - softwarecenter.ui.gtk3.app - INFO - setting up proxy 'None'
2012-06-27 11:05:40,604 - softwarecenter.db.database - INFO - open() database: path=None use_axi=True use_agent=True
2012-06-27 11:05:41,008 - softwarecenter.ui.gtk3.app - INFO - building local database
2012-06-27 11:05:41,008 - softwarecenter.db.pkginfo_impl.aptcache - INFO - aptcache.open()
2012-06-27 11:06:23,961 - softwarecenter.db.database - INFO - open() database: path=None use_axi=True use_agent=True
2012-06-27 11:06:25,197 - softwarecenter.ui.gtk3.app - INFO - show_available_packages: search_text is '', app is None.
2012-06-27 11:06:25,575 - softwarecenter.db.pkginfo_impl.aptcache - INFO - aptcache.open()
2012-06-27 11:06:54,668 - softwarecenter.ui.gtk3.app - INFO - software-center-agent finished with status 0
2012-06-27 11:06:54,671 - softwarecenter.db.database - INFO - reopen() database
2012-06-27 11:06:54,671 - softwarecenter.db.database - INFO - open() database: path=None use_axi=True use_agent=True
2012-06-27 11:07:22,428 - softwarecenter.backend.spawn_helper - WARNING - exit code 1 from helper for '['./data/piston_generic_helper.py', '--datadir', './data', 'SoftwareCenterRecommenderAPI', 'recommend_app', '{"pkgname": "elementals-themagickey"}']'
2012-06-27 11:07:22,428 - softwarecenter.backend.spawn_helper - WARNING - got error from helper: 'WARNING:__main__:404: {'status': '404', 'content-length': '71', 'via': '1.1 papeda.canonical.com:3128 (squid/2.7.STABLE7)', 'x-cache': 'MISS from papeda.canonical.com', 'x-cache-lookup': 'MISS from papeda.canonical.com:3128', 'strict-transport-security': 'max-age=2592000', 'vary': 'Accept-Encoding', 'server': 'Apache/2.2.14 (Ubuntu)', 'etag': '"4f936a9feec02565f3937074a8d3c9e4"', 'cache-control': 'max-age=300', 'date': 'Wed, 27 Jun 2012 10:07:22 GMT', 'content-type': 'text/html; charset=utf-8', '-content-encoding': 'gzip'}
'
2012-06-27 11:07:22,429 - softwarecenter.db.categories - WARNING - Error while accessing the recommender service: WARNING:__main__:404: {'status': '404', 'content-length': '71', 'via': '1.1 papeda.canonical.com:3128 (squid/2.7.STABLE7)', 'x-cache': 'MISS from papeda.canonical.com', 'x-cache-lookup': 'MISS from papeda.canonical.com:3128', 'strict-transport-security': 'max-age=2592000', 'vary': 'Accept-Encoding', 'server': 'Apache/2.2.14 (Ubuntu)', 'etag': '"4f936a9feec02565f3937074a8d3c9e4"', 'cache-control': 'max-age=300', 'date': 'Wed, 27 Jun 2012 10:07:22 GMT', 'content-type': 'text/html; charset=utf-8', '-content-encoding': 'gzip'}

2012-06-27 11:07:22,429 - softwarecenter.ui.gtk3.widgets.recommendations - WARNING - Error while accessing the recommender agent for the details view recommendations: WARNING:__main__:404: {'status': '404', 'content-length': '71', 'via': '1.1 papeda.canonical.com:3128 (squid/2.7.STABLE7)', 'x-cache': 'MISS from papeda.canonical.com', 'x-cache-lookup': 'MISS f...

Read more...

3053. By Gary Lasker on 2012-06-27

tighten the fix

3054. By Gary Lasker on 2012-06-27

simple fix

Revision history for this message
Gary Lasker (gary-lasker) wrote :

I added your test case, thanks mvo! I also simplified the fix (considering the utf8 fix didn't actually work) but just displaying app.pkgname in the offending debug call.

Per your suggestion, I will look for other places where we log an instance of Application and make the same simple fix.

Note that this all is for the SRU cases only. Your fix in lp:~mvo/software-center/fix-lp920741 is really nice and is a much better general fix, so as we've discussed in IRC we should use your fix in trunk for Quantal.

Thanks again!

3055. By Gary Lasker on 2012-06-27

additional fix for logging commercial apps

3056. By Gary Lasker on 2012-06-27

make the app.pkgname fix for DEBUG mode logger calls as well

3057. By Gary Lasker on 2012-06-27

app can be None, handle that

3058. By Gary Lasker on 2012-06-27

add prophylatic checks for app=None where they may be needed, just a further safeguard against regressions

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for this update.

The logging with "app" is only a problem in a context where both string and unicode objects are passed to the logger. In this case it will try to convert the application to a str first (via __str__) and then convert that to unicode. But because there is no __unicode__ method it will assume ascii encoding for the resulting string and fail.

This means that in the places where a single app argument is used its fine and will not crash and its also fine if all strings are of type "str". I mentioned this on irc:

Jun 27 17:15:17 <mvo> tremolux: the only thing that needs invesitgation is if we use a mixed unicode/str in a logger elsewhere that might crash in the same way

but because of the rush to work on the paypal bug I did not put that in the bugreport (I'm sorry for that).

I will revert the parts where there is just a single app argument in the logger and add it to the 5.2 branch
(the added benefit is that the diff also smaller which will make the SRU team happy :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
--- softwarecenter/backend/installbackend_impl/aptd.py 2012-05-21 20:40:57 +0000
+++ softwarecenter/backend/installbackend_impl/aptd.py 2012-06-27 17:23:20 +0000
@@ -550,7 +550,7 @@
550 self._logger.info("add_repo_add_key_and_install_app() '%s' '%s' '%s'" %550 self._logger.info("add_repo_add_key_and_install_app() '%s' '%s' '%s'" %
551 (re.sub("deb https://.*@", "", deb_line), # strip out password551 (re.sub("deb https://.*@", "", deb_line), # strip out password
552 signing_key_id,552 signing_key_id,
553 app))553 app.pkgname))
554554
555 if purchase:555 if purchase:
556 # pre-authenticate556 # pre-authenticate
@@ -618,7 +618,7 @@
618 See _reload_for_commercial_repo_inline() for the actual work618 See _reload_for_commercial_repo_inline() for the actual work
619 that is done619 that is done
620 """620 """
621 self._logger.info("_reload_for_commercial_repo() %s" % app)621 self._logger.info("_reload_for_commercial_repo() %s" % app.pkgname)
622 # trigger inline_callbacked function622 # trigger inline_callbacked function
623 self._reload_for_commercial_repo_defer(623 self._reload_for_commercial_repo_defer(
624 app, trans_metadata, sources_list)624 app, trans_metadata, sources_list)
625625
=== modified file 'softwarecenter/db/application.py'
--- softwarecenter/db/application.py 2012-05-21 20:10:39 +0000
+++ softwarecenter/db/application.py 2012-06-27 17:23:20 +0000
@@ -238,7 +238,8 @@
238 def _on_db_reopen(self, db):238 def _on_db_reopen(self, db):
239 if self._doc:239 if self._doc:
240 try:240 try:
241 LOG.debug("db-reopen, refreshing docid for %s" % self._app)241 LOG.debug("db-reopen, refreshing docid for %s" %
242 self._app.pkgname)
242 self._doc = self._db.get_xapian_document(243 self._doc = self._db.get_xapian_document(
243 self._app.appname, self._app.pkgname)244 self._app.appname, self._app.pkgname)
244 except IndexError:245 except IndexError:
245246
=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
--- softwarecenter/ui/gtk3/panes/availablepane.py 2012-06-08 23:42:50 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-06-27 17:23:20 +0000
@@ -740,7 +740,8 @@
740740
741 def on_application_selected(self, appview, app):741 def on_application_selected(self, appview, app):
742 """callback when an app is selected"""742 """callback when an app is selected"""
743 LOG.debug("on_application_selected: '%s'" % app)743 if app:
744 LOG.debug("on_application_selected: '%s'" % app.pkgname)
744745
745 if self.state.subcategory:746 if self.state.subcategory:
746 self.current_app_by_subcategory[self.state.subcategory] = app747 self.current_app_by_subcategory[self.state.subcategory] = app
@@ -750,7 +751,8 @@
750 @wait_for_apt_cache_ready751 @wait_for_apt_cache_ready
751 def on_application_activated(self, appview, app):752 def on_application_activated(self, appview, app):
752 """callback when an app is clicked"""753 """callback when an app is clicked"""
753 LOG.debug("on_application_activated: '%s'" % app)754 if app:
755 LOG.debug("on_application_activated: '%s'" % app.pkgname)
754 self.state.application = app756 self.state.application = app
755 vm = get_viewmanager()757 vm = get_viewmanager()
756 vm.display_page(self, AvailablePane.Pages.DETAILS, self.state,758 vm.display_page(self, AvailablePane.Pages.DETAILS, self.state,
757759
=== modified file 'softwarecenter/ui/gtk3/panes/installedpane.py'
--- softwarecenter/ui/gtk3/panes/installedpane.py 2012-05-17 12:27:58 +0000
+++ softwarecenter/ui/gtk3/panes/installedpane.py 2012-06-27 17:23:20 +0000
@@ -690,7 +690,8 @@
690690
691 def on_application_selected(self, appview, app):691 def on_application_selected(self, appview, app):
692 """callback when an app is selected"""692 """callback when an app is selected"""
693 logging.debug("on_application_selected: '%s'" % app)693 if app:
694 logging.debug("on_application_selected: '%s'" % app.pkgname)
694 self.current_appview_selection = app695 self.current_appview_selection = app
695696
696 def get_callback_for_page(self, page, state):697 def get_callback_for_page(self, page, state):
697698
=== modified file 'softwarecenter/ui/gtk3/panes/softwarepane.py'
--- softwarecenter/ui/gtk3/panes/softwarepane.py 2012-06-08 21:53:59 +0000
+++ softwarecenter/ui/gtk3/panes/softwarepane.py 2012-06-27 17:23:20 +0000
@@ -259,7 +259,8 @@
259 @wait_for_apt_cache_ready259 @wait_for_apt_cache_ready
260 def on_application_activated(self, appview, app):260 def on_application_activated(self, appview, app):
261 """callback when an app is clicked"""261 """callback when an app is clicked"""
262 LOG.debug("on_application_activated: '%s'" % app)262 if app:
263 LOG.debug("on_application_activated: '%s'" % app.pkgname)
263264
264 self.state.application = app265 self.state.application = app
265266
266267
=== modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py'
--- softwarecenter/ui/gtk3/views/appdetailsview.py 2012-06-18 14:39:27 +0000
+++ softwarecenter/ui/gtk3/views/appdetailsview.py 2012-06-27 17:23:20 +0000
@@ -984,7 +984,7 @@
984 """ callback when new reviews are ready, cleans out the984 """ callback when new reviews are ready, cleans out the
985 old ones985 old ones
986 """986 """
987 LOG.debug("_review_ready_callback: %s" % app)987 LOG.debug("_review_ready_callback: %s" % app.pkgname)
988 # avoid possible race if we already moved to a new app when988 # avoid possible race if we already moved to a new app when
989 # the reviews become ready989 # the reviews become ready
990 # (we only check for pkgname currently to avoid breaking on990 # (we only check for pkgname currently to avoid breaking on
@@ -1697,10 +1697,11 @@
16971697
1698 # public API1698 # public API
1699 def show_app(self, app, force=False):1699 def show_app(self, app, force=False):
1700 LOG.debug("AppDetailsView.show_app '%s'" % app)
1701 if app is None:1700 if app is None:
1702 LOG.debug("no app selected")1701 LOG.debug("no app selected")
1703 return1702 return
1703 else:
1704 LOG.debug("AppDetailsView.show_app '%s'" % app.pkgname)
17041705
1705 same_app = (self.app and1706 same_app = (self.app and
1706 self.app.pkgname and1707 self.app.pkgname and
17071708
=== modified file 'test/test_aptd.py'
--- test/test_aptd.py 2012-01-16 14:42:49 +0000
+++ test/test_aptd.py 2012-06-27 17:23:20 +0000
@@ -1,4 +1,5 @@
1#!/usr/bin/python1#!/usr/bin/python
2# -*- coding: utf-8 -*-
23
3import os4import os
4import time5import time
@@ -7,6 +8,7 @@
7from testutils import setup_test_env8from testutils import setup_test_env
8setup_test_env()9setup_test_env()
910
11from softwarecenter.db.application import Application
10from softwarecenter.backend.installbackend_impl.aptd import AptdaemonBackend12from softwarecenter.backend.installbackend_impl.aptd import AptdaemonBackend
11from defer import inline_callbacks13from defer import inline_callbacks
12from mock import Mock14from mock import Mock
@@ -99,6 +101,21 @@
99 addons_remove = ["gimp-plugin-registry"]101 addons_remove = ["gimp-plugin-registry"]
100 yield self.aptd.apply_changes(pkgname, appname ,iconname, addons_install, addons_remove)102 yield self.aptd.apply_changes(pkgname, appname ,iconname, addons_install, addons_remove)
101103
104 @inline_callbacks
105 def _inline_add_repo_call(self):
106 deb_line = "deb https://foo"
107 signing_key_id = u"xxx"
108 app = Application(u"Elementals: The Magic Key™", "pkgname")
109 iconname = "iconname"
110 yield self.aptd.add_repo_add_key_and_install_app(
111 deb_line, signing_key_id, app, iconname, None, None)
112
113 def test_add_repo_add_key_and_install_app(self):
114 from mock import patch
115 with patch.object(self.aptd._logger, "info") as mock:
116 self._inline_add_repo_call()
117 self.assertTrue(
118 mock.call_args[0][0].startswith("add_repo_add_key"))
102119
103if __name__ == "__main__":120if __name__ == "__main__":
104 import logging121 import logging

Subscribers

People subscribed via source and target branches