click

Merge lp:~mvo/click/dont-crash-for-empty-db into lp:click/devel

  1. dont-crash-for-empty-db
  2. Merge into devel
Proposed by Michael Vogt
Status: Merged
Approved by: Michael Vogt
Approved revision: 479
Merged at revision: 560
Proposed branch: lp:~mvo/click/dont-crash-for-empty-db
Merge into: lp:click/devel
Diff against target: 102 lines (+41/-3)
2 files modified
click/tests/test_database.py (+12/-0)
lib/click/database.vala (+29/-3)
To merge this branch: bzr merge lp:~mvo/click/dont-crash-for-empty-db
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
click hackers Pending
Review via email: mp+224318@code.launchpad.net

Commit message

Do not crash when no database configuration is used and Click.DB.{get,props.overlay,gc,ensure_ownership} are called.

Description of the change

This branch adds checks into libclick to avoid crashing if no database configuration is loaded.

This is a better version of the (rightfully) reverted r474. I currently have a single error type for this. If you think the case "no database at all" and "invalid index in get()" should be seperate error types I'm happy to do that.

AIUI this is a ABI break as get() now throws a error when it wasn't before (i.e. a GError is now part of the get function signature). So we may need to defer this until the next abi break or return "null" or something instead of raising a error.

Thanks,
 Michael

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:478
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mvo/click/dont-crash-for-empty-db/+merge/224318/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/click-devel-ci/4/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/4/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/4

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/4/rebuild

review: Needs Fixing (continuous-integration)
lp:~mvo/click/dont-crash-for-empty-db updated
479. By Michael Vogt

merged lp:click/devel

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:479
http://jenkins.qa.ubuntu.com/job/click-devel-ci/80/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-amd64-ci/82
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/80
        deb: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-armhf-ci/80/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/click-devel-utopic-i386-ci/80

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/click-devel-ci/80/rebuild

review: Approve (continuous-integration)
Revision history for this message
Barry Warsaw (barry) wrote :

>This is a better version of the (rightfully) reverted r474. I currently have a single error >type for this. If you think the case "no database at all" and "invalid index in get()" should >be seperate error types I'm happy to do that.

Would a typical user be able to do something different in these two cases? If so then having separate errors makes sense. If not, then it probably doesn't effectively matter (except perhaps for API consistency, or future proofing, but OTOH YAGNI?).

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

Thanks for the review! I'm in favor of YAGNI in this case :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'click/tests/test_database.py'
--- click/tests/test_database.py 2014-09-10 11:54:14 +0000
+++ click/tests/test_database.py 2014-09-29 11:41:01 +0000
@@ -562,6 +562,18 @@
562 db = Click.DB()562 db = Click.DB()
563 self.assertEqual(0, db.props.size)563 self.assertEqual(0, db.props.size)
564564
565 def test_no_db_conf_errors(self):
566 db = Click.DB()
567 self.assertRaisesDatabaseError(
568 Click.DatabaseError.INVALID, db.get, 0)
569 self.assertEqual(db.props.overlay, "")
570 self.assertRaisesDatabaseError(
571 Click.DatabaseError.INVALID, db.maybe_remove, "something", "1.0")
572 self.assertRaisesDatabaseError(
573 Click.DatabaseError.INVALID, db.gc)
574 self.assertRaisesDatabaseError(
575 Click.DatabaseError.INVALID, db.ensure_ownership)
576
565 def test_read_nonexistent(self):577 def test_read_nonexistent(self):
566 db = Click.DB()578 db = Click.DB()
567 db.read(db_dir=os.path.join(self.temp_dir, "nonexistent"))579 db.read(db_dir=os.path.join(self.temp_dir, "nonexistent"))
568580
=== modified file 'lib/click/database.vala'
--- lib/click/database.vala 2014-09-12 11:46:05 +0000
+++ lib/click/database.vala 2014-09-29 11:41:01 +0000
@@ -34,7 +34,11 @@
34 /**34 /**
35 * Package manifest cannot be parsed.35 * Package manifest cannot be parsed.
36 */36 */
37 BAD_MANIFEST37 BAD_MANIFEST,
38 /**
39 * No database available for the given request
40 */
41 INVALID
38}42}
3943
40private string? app_pid_command = null;44private string? app_pid_command = null;
@@ -602,8 +606,11 @@
602 public int size { get { return db.size; } }606 public int size { get { return db.size; } }
603607
604 public new SingleDB608 public new SingleDB
605 @get (int index)609 @get (int index) throws DatabaseError
606 {610 {
611 if (index >= db.size)
612 throw new DatabaseError.INVALID
613 ("invalid index %i for db of size %i", index, db.size);
607 return db.get (index);614 return db.get (index);
608 }615 }
609616
@@ -618,7 +625,15 @@
618 *625 *
619 * The directory where changes should be written.626 * The directory where changes should be written.
620 */627 */
621 public string overlay { get { return db.last ().root; } }628 public string overlay
629 {
630 get {
631 if (db.size == 0)
632 return "";
633 else
634 return db.last ().root;
635 }
636 }
622637
623 /**638 /**
624 * get_path:639 * get_path:
@@ -812,21 +827,32 @@
812 return generator.to_data (null);827 return generator.to_data (null);
813 }828 }
814829
830 private void
831 ensure_db () throws Error
832 {
833 if (db.size == 0)
834 throw new DatabaseError.INVALID
835 ("no database loaded");
836 }
837
815 public void838 public void
816 maybe_remove (string package, string version) throws Error839 maybe_remove (string package, string version) throws Error
817 {840 {
841 ensure_db();
818 db.last ().maybe_remove (package, version);842 db.last ().maybe_remove (package, version);
819 }843 }
820844
821 public void845 public void
822 gc () throws Error846 gc () throws Error
823 {847 {
848 ensure_db();
824 db.last ().gc ();849 db.last ().gc ();
825 }850 }
826851
827 public void852 public void
828 ensure_ownership () throws Error853 ensure_ownership () throws Error
829 {854 {
855 ensure_db();
830 db.last ().ensure_ownership ();856 db.last ().ensure_ownership ();
831 }857 }
832}858}

Subscribers

People subscribed via source and target branches

to all changes: