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

[Michael Vogt]

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

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

[PS Jenkins bot (ps-jenkins)]

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]

479. By Michael Vogt on 2014-09-29

merged lp:click/devel

[PS Jenkins bot (ps-jenkins)]

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)

[Barry Warsaw (barry)]

>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?).

[Michael Vogt (mvo)]

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

=== 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 @@
         db = Click.DB()
         self.assertEqual(0, db.props.size)
 
+    def test_no_db_conf_errors(self):
+        db = Click.DB()
+        self.assertRaisesDatabaseError(
+            Click.DatabaseError.INVALID, db.get, 0)
+        self.assertEqual(db.props.overlay, "")
+        self.assertRaisesDatabaseError(
+            Click.DatabaseError.INVALID, db.maybe_remove, "something", "1.0")
+        self.assertRaisesDatabaseError(
+            Click.DatabaseError.INVALID, db.gc)
+        self.assertRaisesDatabaseError(
+            Click.DatabaseError.INVALID, db.ensure_ownership)
+
     def test_read_nonexistent(self):
         db = Click.DB()
         db.read(db_dir=os.path.join(self.temp_dir, "nonexistent"))

=== 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 @@
 	/**
 	 * Package manifest cannot be parsed.
 	 */
-	BAD_MANIFEST
+	BAD_MANIFEST,
+	/**
+	 * No database available for the given request
+	 */
+	INVALID
 }
 
 private string? app_pid_command = null;
@@ -602,8 +606,11 @@
 	public int size { get { return db.size; } }
 
 	public new SingleDB
-	@get (int index)
+	@get (int index) throws DatabaseError
 	{
+		if (index >= db.size)
+			throw new DatabaseError.INVALID
+                ("invalid index %i for db of size %i", index, db.size);
 		return db.get (index);
 	}
 
@@ -618,7 +625,15 @@
 	 *
 	 * The directory where changes should be written.
 	 */
-	public string overlay { get { return db.last ().root; } }
+	public string overlay 
+	{ 
+		get {
+			if (db.size == 0)
+				return "";
+			else
+				return db.last ().root;
+		}
+	}
 
 	/**
 	 * get_path:
@@ -812,21 +827,32 @@
 		return generator.to_data (null);
 	}
 
+	private void
+	ensure_db () throws Error
+	{
+		if (db.size == 0)
+			throw new DatabaseError.INVALID
+                ("no database loaded");
+	}
+
 	public void
 	maybe_remove (string package, string version) throws Error
 	{
+		ensure_db();
 		db.last ().maybe_remove (package, version);
 	}
 
 	public void
 	gc () throws Error
 	{
+		ensure_db();
 		db.last ().gc ();
 	}
 
 	public void
 	ensure_ownership () throws Error
 	{
+		ensure_db();
 		db.last ().ensure_ownership ();
 	}
 }