Always report the update-error correctly for multiple updates

The root of this problem is that we were trying to use pending.db as both a
pending report store and also a record of history, so you could see updates for
device foo 1.2.3->1.2.4 and then 1.2.4->1.2.5.
This sort-of half worked, but it meant in some cases we matched on the old
device version, sometimes on the new release version and sometimes matching on
either.

The problem was that in various places we were using the device ID as *the*
primary key, and so we had various functions like fu_history_get_device_by_id()
that expected to return just one device, not several.

The device ID also changes if the device is moved from USB plug to another, and
so it got even more complicated again trying to de-dupe the devices.

The sanest thing to do is to decide that pending.db only contains the latest
attempt to go from one version to another using device-id as the primary key.
This makes reporting work reliably now, although has the effect you can't
report a 1.2.3->1.2.4 and 1.2.4->1.2.5 at the same time. Note, if you tried to
do the latter before, the update-error (if set) would be wrong on the second
report...

For the 1.3.3 release make it all simpler and so the reporting works reliably.
Longer term we can design a better database schema that uses unique ID to
represent the *transaction* itself, that isn't tied to the device ID in any way.
This might mean extending the DBus API to cope with multiple devices being
returned with the same device-id set.

src/fu-engine.c

@@ -660,10 +660,7 @@ fu_engine_modify_device (FuEngine *self,
 			return FALSE;
 		}
 		fu_device_add_flag (device, flag);
-		return fu_history_modify_device (self->history, device,
-						 FU_HISTORY_FLAGS_MATCH_OLD_VERSION |
-						 FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-						 error);
+		return fu_history_modify_device (self->history, device, error);
 	}
 
 	/* others invalid */
@@ -1739,10 +1736,7 @@ fu_engine_install (FuEngine *self,
 		}
 		fu_device_set_update_error (device, error_local->message);
 		if ((flags & FWUPD_INSTALL_FLAG_NO_HISTORY) == 0 &&
-		    !fu_history_modify_device (self->history, device,
-					       FU_HISTORY_FLAGS_MATCH_OLD_VERSION |
-					       FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-					       error)) {
+		    !fu_history_modify_device (self->history, device, error)) {
 			return FALSE;
 		}
 		g_propagate_error (error, g_steal_pointer (&error_local));
@@ -1764,10 +1758,7 @@ fu_engine_install (FuEngine *self,
 	    fu_device_has_flag (device, FWUPD_DEVICE_FLAG_NEEDS_SHUTDOWN)) {
 		fu_device_set_update_state (device, FWUPD_UPDATE_STATE_NEEDS_REBOOT);
 		if ((flags & FWUPD_INSTALL_FLAG_NO_HISTORY) == 0 &&
-		    !fu_history_modify_device (self->history, device,
-					       FU_HISTORY_FLAGS_MATCH_OLD_VERSION |
-					       FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-					       error))
+		    !fu_history_modify_device (self->history, device, error))
 			return FALSE;
 		/* success */
 		return TRUE;
@@ -1783,9 +1774,7 @@ fu_engine_install (FuEngine *self,
 				       version_rel, fu_device_get_version (device));
 		fu_device_set_update_error (device, str);
 		if ((flags & FWUPD_INSTALL_FLAG_NO_HISTORY) == 0 &&
-		    !fu_history_modify_device (self->history, device,
-					       FU_HISTORY_FLAGS_MATCH_OLD_VERSION,
-					       error))
+		    !fu_history_modify_device (self->history, device, error))
 			return FALSE;
 		/* success */
 		return TRUE;
@@ -1803,9 +1792,7 @@ fu_engine_install (FuEngine *self,
 	/* success */
 	fu_device_set_update_state (device, FWUPD_UPDATE_STATE_SUCCESS);
 	if ((flags & FWUPD_INSTALL_FLAG_NO_HISTORY) == 0 &&
-	    !fu_history_modify_device (self->history, device,
-				       FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-				       error))
+	    !fu_history_modify_device (self->history, device, error))
 		return FALSE;
 	return TRUE;
 }
@@ -3636,10 +3623,7 @@ fu_engine_clear_results (FuEngine *self, const gchar *device_id, GError **error)
 
 	/* override */
 	fu_device_add_flag (device, FWUPD_DEVICE_FLAG_NOTIFIED);
-	return fu_history_modify_device (self->history, device,
-					 FU_HISTORY_FLAGS_MATCH_OLD_VERSION |
-					 FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-					 error);
+	return fu_history_modify_device (self->history, device, error);
 }
 
 /**
@@ -4598,9 +4582,7 @@ fu_engine_update_history_device (FuEngine *self, FuDevice *dev_history, GError *
 				       fu_device_get_version_format (dev));
 		fu_device_remove_flag (dev_history, FWUPD_DEVICE_FLAG_NEEDS_ACTIVATION);
 		fu_device_set_update_state (dev_history, FWUPD_UPDATE_STATE_SUCCESS);
-		return fu_history_modify_device (self->history, dev_history,
-						 FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-						 error);
+		return fu_history_modify_device (self->history, dev_history, error);
 	}
 
 	/* does the plugin know the update failure */
@@ -4621,9 +4603,7 @@ fu_engine_update_history_device (FuEngine *self, FuDevice *dev_history, GError *
 
 	/* update the state in the database */
 	fu_device_set_update_error (dev_history, fu_device_get_update_error (dev));
-	return fu_history_modify_device (self->history, dev_history,
-					 FU_HISTORY_FLAGS_MATCH_OLD_VERSION,
-					 error);
+	return fu_history_modify_device (self->history, dev_history, error);
 }
 
 static gboolean

src/fu-history.c

@@ -458,9 +458,7 @@ fu_history_get_device_flags_filtered (FuDevice *device)
 }
 
 gboolean
-fu_history_modify_device (FuHistory *self, FuDevice *device,
-			  FuHistoryFlags flags,
-			  GError **error)
+fu_history_modify_device (FuHistory *self, FuDevice *device, GError **error)
 {
 	gint rc;
 	g_autoptr(sqlite3_stmt) stmt = NULL;
@@ -476,51 +474,18 @@ fu_history_modify_device (FuHistory *self, FuDevice *device,
 	/* overwrite entry if it exists */
 	locker = g_rw_lock_writer_locker_new (&self->db_mutex);
 	g_return_val_if_fail (locker != NULL, FALSE);
-	if ((flags & FU_HISTORY_FLAGS_MATCH_OLD_VERSION) &&
-	    (flags & FU_HISTORY_FLAGS_MATCH_NEW_VERSION)) {
-		g_debug ("modifying device %s [%s], version not important",
-			 fu_device_get_name (device),
-			 fu_device_get_id (device));
-		rc = sqlite3_prepare_v2 (self->db,
-					 "UPDATE history SET "
-					 "update_state = ?1, "
-					 "update_error = ?2, "
-					 "checksum_device = ?6, "
-					 "device_modified = ?7, "
-					 "flags = ?3 "
-					 "WHERE device_id = ?4;",
-					 -1, &stmt, NULL);
-	} else if (flags & FU_HISTORY_FLAGS_MATCH_OLD_VERSION) {
-		g_debug ("modifying device %s [%s], only version old %s",
-			 fu_device_get_name (device),
-			 fu_device_get_id (device),
-			 fu_device_get_version (device));
-		rc = sqlite3_prepare_v2 (self->db,
-					 "UPDATE history SET "
-					 "update_state = ?1, "
-					 "update_error = ?2, "
-					 "checksum_device = ?6, "
-					 "device_modified = ?7, "
-					 "flags = ?3 "
-					 "WHERE device_id = ?4 AND version_old = ?5;",
-					 -1, &stmt, NULL);
-	} else if (flags & FU_HISTORY_FLAGS_MATCH_NEW_VERSION) {
-		g_debug ("modifying device %s [%s], only version new %s",
-			 fu_device_get_name (device),
-			 fu_device_get_id (device),
-			 fu_device_get_version (device));
-		rc = sqlite3_prepare_v2 (self->db,
-					 "UPDATE history SET "
-					 "update_state = ?1, "
-					 "update_error = ?2, "
-					 "checksum_device = ?6, "
-					 "device_modified = ?7, "
-					 "flags = ?3 "
-					 "WHERE device_id = ?4 AND version_new = ?5;",
-					 -1, &stmt, NULL);
-	} else {
-		g_assert_not_reached ();
-	}
+	g_debug ("modifying device %s [%s]",
+		 fu_device_get_name (device),
+		 fu_device_get_id (device));
+	rc = sqlite3_prepare_v2 (self->db,
+				 "UPDATE history SET "
+				 "update_state = ?1, "
+				 "update_error = ?2, "
+				 "checksum_device = ?6, "
+				 "device_modified = ?7, "
+				 "flags = ?3 "
+				 "WHERE device_id = ?4;",
+				 -1, &stmt, NULL);
 	if (rc != SQLITE_OK) {
 		g_set_error (error, FWUPD_ERROR, FWUPD_ERROR_INTERNAL,
 			     "Failed to prepare SQL to update history: %s",
@@ -558,10 +523,9 @@ fu_history_add_device (FuHistory *self, FuDevice *device, FwupdRelease *release,
 	if (!fu_history_load (self, error))
 		return FALSE;
 
-	/* ensure device with this old-version -> new-version does not exist */
-	if (!fu_history_remove_device (self, device, release, error))
+	/* ensure all old device(s) with this ID are removed */
+	if (!fu_history_remove_device (self, device, error))
 		return FALSE;
-
 	g_debug ("add device %s [%s]",
 		 fu_device_get_name (device),
 		 fu_device_get_id (device));
@@ -683,8 +647,7 @@ fu_history_remove_all (FuHistory *self, GError **error)
 }
 
 gboolean
-fu_history_remove_device (FuHistory *self,  FuDevice *device,
-			  FwupdRelease *release, GError **error)
+fu_history_remove_device (FuHistory *self,  FuDevice *device, GError **error)
 {
 	gint rc;
 	g_autoptr(sqlite3_stmt) stmt = NULL;
@@ -692,7 +655,6 @@ fu_history_remove_device (FuHistory *self,  FuDevice *device,
 
 	g_return_val_if_fail (FU_IS_HISTORY (self), FALSE);
 	g_return_val_if_fail (FU_IS_DEVICE (device), FALSE);
-	g_return_val_if_fail (FWUPD_IS_RELEASE (release), FALSE);
 
 	/* lazy load */
 	if (!fu_history_load (self, error))
@@ -704,9 +666,7 @@ fu_history_remove_device (FuHistory *self,  FuDevice *device,
 		 fu_device_get_name (device),
 		 fu_device_get_id (device));
 	rc = sqlite3_prepare_v2 (self->db,
-				 "DELETE FROM history WHERE device_id = ?1 "
-				 "AND version_old = ?2 "
-				 "AND version_new = ?3;",
+				 "DELETE FROM history WHERE device_id = ?1;",
 				 -1, &stmt, NULL);
 	if (rc != SQLITE_OK) {
 		g_set_error (error, FWUPD_ERROR, FWUPD_ERROR_INTERNAL,
@@ -715,8 +675,6 @@ fu_history_remove_device (FuHistory *self,  FuDevice *device,
 		return FALSE;
 	}
 	sqlite3_bind_text (stmt, 1, fu_device_get_id (device), -1, SQLITE_STATIC);
-	sqlite3_bind_text (stmt, 2, fu_device_get_version (device), -1, SQLITE_STATIC);
-	sqlite3_bind_text (stmt, 3, fwupd_release_get_version (release), -1, SQLITE_STATIC);
 	return fu_history_stmt_exec (self, stmt, NULL, error);
 }
 

src/fu-history.h

@@ -13,22 +13,6 @@
 #define FU_TYPE_PENDING (fu_history_get_type ())
 G_DECLARE_FINAL_TYPE (FuHistory, fu_history, FU, HISTORY, GObject)
 
-/**
- * FuHistoryFlags:
- * @FU_HISTORY_FLAGS_NONE:			No flags set
- * @FU_HISTORY_FLAGS_MATCH_OLD_VERSION:		Match previous firmware version
- * @FU_HISTORY_FLAGS_MATCH_NEW_VERSION:		Match new firmware version
- *
- * The flags to use when matching devices against the history database.
- **/
-typedef enum {
-	FU_HISTORY_FLAGS_NONE			= 0,
-	FU_HISTORY_FLAGS_MATCH_OLD_VERSION	= 1 << 0,
-	FU_HISTORY_FLAGS_MATCH_NEW_VERSION	= 1 << 1,
-	/*< private >*/
-	FU_HISTORY_FLAGS_LAST
-} FuHistoryFlags;
-
 FuHistory	*fu_history_new				(void);
 
 gboolean	 fu_history_add_device			(FuHistory	*self,
@@ -37,11 +21,9 @@ gboolean	 fu_history_add_device			(FuHistory	*self,
 							 GError		**error);
 gboolean	 fu_history_modify_device		(FuHistory	*self,
 							 FuDevice	*device,
-							 FuHistoryFlags	 flags,
 							 GError		**error);
 gboolean	 fu_history_remove_device		(FuHistory	*self,
 							 FuDevice	*device,
-							 FwupdRelease	*release,
 							 GError		**error);
 gboolean	 fu_history_remove_all			(FuHistory	*self,
 							 GError		**error);

src/fu-plugin.c

@@ -1948,9 +1948,7 @@ fu_plugin_runner_update (FuPlugin *self,
 
 		/* update history database */
 		fu_device_set_update_state (device, FWUPD_UPDATE_STATE_SUCCESS);
-		if (!fu_history_modify_device (history, device,
-					       FU_HISTORY_FLAGS_MATCH_NEW_VERSION,
-					       error))
+		if (!fu_history_modify_device (history, device, error))
 			return FALSE;
 
 		/* delete cab file */

src/fu-self-test.c

@@ -2542,7 +2542,7 @@ fu_history_func (void)
 	g_object_unref (device_found);
 
 	/* remove device */
-	ret = fu_history_remove_device (history, device, release, &error);
+	ret = fu_history_remove_device (history, device, &error);
 	g_assert_no_error (error);
 	g_assert (ret);
 	g_object_unref (device);