Gladys V4 - Affichage de capteurs dans l'interface

damalgos · Septembre 23, 2019, 7:32

Bonjour à tous,

Je voulais poster un message suite à un soucis que j’ai sur le module xiaomi.

En effet lorsque l’on récupère un capteurs, il sera rajouté et visible sur la vue par exemple “device”. Le problème est le nommage de ces capteurs. En gros le nom doit être unique. Cependant c’est idiot de mettre un nom unique alors que dans chaque pièce le capteur peut très bien s’appeller “capteur de température”. Sauf que actuellement c’est un nom unique donc ca se transforme en capteur de la pièce machine ou capteur 1 …

Serait-il pas plus intéressant de mettre en place un nommage automatique qui prend en compte le type de capteur et qui affiche un nommage particulier ? Comme ça au niveau dev on a juste a rajouter le device et mettre le nom que l’on veut et c’est le front qui gère tout seul le nommage.

Qu’en pensez vous ?

AlexTrovato · Septembre 23, 2019, 8:41

Salut,
Le nom unique je trouve ça aussi très limitatif, ayant également plusieurs capteurs de température.
Ce nom devrait être modifiable par l’utilisateur et ne devrait servir que de libellé, et non de valeur fonctionnelle.
Maintenant qu’il soit généré automatiquement, je pense que ça risque de faire des noms à dormir debout.

damalgos · Septembre 23, 2019, 9:16

Non je pense que tu as pas complèteent compris.

Logiquement dans ta pièce tu as 1 capteur de température. Etant donné qu’il est dans cette pièce il peut avoir le nom de Capteur de température. Ce nom peut être présent plusieurs fois car unique pour chaque pièce.

Si jamais tu as deux capteurs tu les nommes 1 - 2.

La génération se fait à la volée et ne sera logiquement pas stocké en base de données.

La seconde idée est de définir une nouvelle variable qui n’est pas le nom et qui n’est pas unique. Celle-ci permettra d’avoir plusieurs “Capteur de température” avec le même nom si l’utilisateur le souhaite.

En soit tu regardes d’autres système, on définit aucun nom, on rajoute juste un équipement à une pièce.

pierre-gilles · Septembre 23, 2019, 3:06

@damalgos: Je vais retirer la contrainte d’unicité sur le nom, ça n’a pas de sens De toute façon, de la façon dont on a commencé à développer les services dans Gladys 4, le “name” ne sert à rien et n’est pas utilisé

damalgos · Septembre 23, 2019, 3:20

Et bah ça c’est une bonne nouvelle

pierre-gilles · Septembre 24, 2019, 7:29

Bon je me suis rendu compte que SQLite ne permettait pas de retirer les index unique lors d’une migration, du coup c’est pas aussi simple que ça ^^

Si on avait été en production release, on aurait du:

Faire sauter temporairement les foreign key pour le migration
Renommer la table t_device en t_device_old
Créer la nouvelle table t_device
Transférer les données de l’ancienne à la nouvelle
Supprimer l’ancienne
Remettre les foreign key

Etant donné qu’on est qu’en alpha et qu’il n’y a pas encore de donnée sensible chez les utilisateurs, l’approche la plus simple côté développeur (histoire de pas perdre de temps), c’est de modifier exceptionnellement directement les migrations initiales, ce qui veut dire que tous ceux qui ont installé l’alpha devront perdre leurs données et recréer une DB!

Je vais en profiter pour faire une passe sur toute la DB pouvoir si il n’y a pas d’autres contraintes inutiles à faire sauter. J’ai vu que le calendrier et les event ont le même soucis, je les retire aussi.

Pas super marrant, mais bon c’est une alpha. Bien entendu ce genre de procédure ne sera jamais faites une fois qu’on sera en production…

bertrandda · Septembre 26, 2019, 8:30

Hello @Pierre-Gilles, j’ai eu la même difficulté que toi pour retirer l’unicité de la base, mais finalement c’est possible. Regarde sur la PR CalDAV : https://github.com/GladysAssistant/Gladys/pull/507/commits/24e95947bbad98c23856e7a36491102916b6c054#diff-875173103ade96e5af1bdede5b6ae683R3

pierre-gilles · Septembre 27, 2019, 7:05

Es-tu sur que ça fonctionne ? D’après mes recherches et mes tests, SQlite ne permet pas de supprimer une contrainte d’unicité, il faut passer par une création + migration de table… Après je me trompe peut être, dis moi

bertrandda · Septembre 27, 2019, 7:46

Oui cela fonctionne très bien, j’avais un problème avec l’unicité sur les noms d’évènements récurrents, avec ça on peut maintenant enregistrer autant d’évènements Anniversaire de Pepper que l’on veut.

pierre-gilles · Septembre 28, 2019, 12:37

Ok merci pour ton retour! Je vais tester ça

pierre-gilles · Septembre 30, 2019, 9:09

@bertrandda malheureusement ta solution ne fonctionne pas à 100%, en fait tu as mis le doigt sur un bug (ou une feature^^) de sequelize!

En SQlite ALTER TABLE n’existe pas. La seule façon de mettre à jour une table est de la renommer, de la recréer et de migrer les données.

Sequelize a abstrait cette partie, et fait ça quand tu fais ton changeColumn:

Executing (default): PRAGMA TABLE_INFO(`t_device`);
Executing (default): CREATE TABLE IF NOT EXISTS `t_device_backup` (`id` UUID NOT NULL PRIMARY KEY, `service_id` UUID NOT NULL, `room_id` UUID, `name` VARCHAR(255) NOT NULL, `selector` VARCHAR(255) NOT NULL, `model` VARCHAR(255), `external_id` VARCHAR(255) NOT NULL, `should_poll` TINYINT(1) NOT NULL DEFAULT 0, `poll_frequency` INTEGER, `created_at` DATETIME NOT NULL, `updated_at` DATETIME NOT NULL);
Executing (default): INSERT INTO `t_device_backup` SELECT `id`, `service_id`, `room_id`, `name`, `selector`, `model`, `external_id`, `should_poll`, `poll_frequency`, `created_at`, `updated_at` FROM `t_device`;
Executing (default): DROP TABLE `t_device`;
Executing (default): CREATE TABLE IF NOT EXISTS `t_device` (`id` UUID NOT NULL PRIMARY KEY, `service_id` UUID NOT NULL, `room_id` UUID, `name` VARCHAR(255) NOT NULL, `selector` VARCHAR(255) NOT NULL, `model` VARCHAR(255), `external_id` VARCHAR(255) NOT NULL, `should_poll` TINYINT(1) NOT NULL DEFAULT 0, `poll_frequency` INTEGER, `created_at` DATETIME NOT NULL, `updated_at` DATETIME NOT NULL);
Executing (default): INSERT INTO `t_device` SELECT `id`, `service_id`, `room_id`, `name`, `selector`, `model`, `external_id`, `should_poll`, `poll_frequency`, `created_at`, `updated_at` FROM `t_device_backup`;
Executing (default): DROP TABLE `t_device_backup`;

Or, si tu lis bien le code, il y a un bug dans cette implémentation: les foreign key + les contraints ne sont pas migrées. Donc oui, ton name n’a plus de contraintes: mais toutes les autres columns ont aussi perdu leurs contraintes aussi, et la table se retrouve sans aucunes protections. Le selector n’est plus unique, les foreign keys ont sautés, etc…

Cf code :

github.com

sequelize/sequelize/blob/efd2f40620107cef01e56856457ba1b607af79f5/lib/dialects/sqlite/query-interface.js#L46

    
      
          
          
    const sql = this.QueryGenerator.removeColumnQuery(tableName, fields);
              const subQueries = sql.split(';').filter(q => q !== '');
          
          
    return Promise.each(subQueries, subQuery => this.sequelize.query(subQuery + ';', _.assign({raw: true}, options)));
            });
          }
          exports.removeColumn = removeColumn;
          
          
/**
            A wrapper that fixes SQLite's inability to change columns from existing tables.
            It will create a backup of the table, drop the table afterwards and create a
            new table with the same name but with a modified version of the respective column.
          
          
  @method changeColumn
            @for    QueryInterface
          
          
  @param  {String} tableName The name of the table.
            @param  {Object} attributes An object with the attribute's name as key and its options as value object.
            @param  {Object} options
            @param  {Boolean|Function} [options.logging] A function that logs the sql queries, or false for explicitly not logging these queries

Après fouille du GitHub de Sequelize, il y a une issue créé en 2017 qui référence le problème:

github.com/sequelize/sequelize

Sqlite remove/change/rename column don't pass foreign key references along in the new table

opened 12:33PM - 06 Jan 17 UTC

closed 04:22AM - 28 Jun 20 UTC

francescoinfante

type: bug type: refactor released status: awaiting investigation

## What you are doing? I'm trying to rename a column on an Sqlite db. I use `re…nameColumn` on the `QueryInterface` that, in the Sqlite case, is going to call `renameColumn` on the `SQLiteQueryInterface`. ## What do you expect to happen? I expect that: 1) a temporary table is created equal to the first one (except the renamed column) including foreign key references. 2) data is copied over to the temporary table 3) old table is deleted 4) a new table is created equal to the first one (except the renamed column) including foreign key references. 5) data is copied over to the new table ## What is actually happening? 1) a temporary table is created equal to the first one (except the renamed column) **NOT including foreign key references**. 2) data is copied over to the temporary table 3) old table is deleted 4) a new table is created equal to the first one (except the renamed column) **NOT including foreign key references**. 5) data is copied over to the new table This is the original table: ```SQL CREATE TABLE `matches` (`id` INTEGER PRIMARY KEY AUTOINCREMENT, `hash` VARCHAR(200) NOT NULL UNIQUE, `alertId` INTEGER REFERENCES `alerts` (`id`) ON DELETE CASCADE ON UPDATE CASCADE) ``` This is the table after the renameColumn on 'hash' -> 'foo' ```SQL CREATE TABLE `matches` (`id` INTEGER PRIMARY KEY AUTOINCREMENT, `foo` VARCHAR(200) NOT NULL, `alertId` INTEGER) ``` __Dialect:__ sqlite __Database version:__ 3.9.x __Sequelize version:__ 3.24.x

Mais visiblement le bug n’est toujours pas résolu…

Trois options:

On utilise un “hack” qui considère à override une des fonctions de Sequelize lors de cette migration pour re-rajouter les contraintes.
On trouve un moyen de fixer Sequelize et on propose une PR (après pas sur que ça soit mergé avant qu’on en ait besoin)
On oublie de faire un changeColumn et on demande aux gens de re-créer la DB… ce qui fixe à court terme, mais bon si à long terme on veut changer une column le problème est toujours là !

pierre-gilles · Septembre 30, 2019, 10:00

J’ai réussi à faire le hack, voilà ce que ça donne:

module.exports = {
  up: async (queryInterface, Sequelize) => {
    const backupDescribeTable = queryInterface.describeTable;
    queryInterface.describeTable = async (tableName, options) => {
      const tableDatas = await backupDescribeTable.call(queryInterface, tableName, options);
      // get foreign key info
      const foreignKeyDatas = (await queryInterface.sequelize.query(`PRAGMA FOREIGN_KEY_LIST("${tableName}");`))[0];
      foreignKeyDatas.forEach((foreignKeyData) => {
        const tableData = tableDatas[foreignKeyData.from];
        tableData.references = { model: foreignKeyData.table, key: foreignKeyData.to };
        tableData.onDelete = foreignKeyData.on_delete;
        tableData.onUpdate = foreignKeyData.on_update;
      });
      if (tableName === 't_device') {
        tableDatas.selector.unique = true;
        tableDatas.external_id.unique = true;
      }
      return tableDatas;
    };
    await queryInterface.changeColumn('t_device', 'name', {
      allowNull: false,
      type: Sequelize.STRING,
      unique: false,
    });
    queryInterface.describeTable = backupDescribeTable;
  },

  down: async (queryInterface, Sequelize) => {
    const backupDescribeTable = queryInterface.describeTable;
    queryInterface.describeTable = async (tableName, options) => {
      const tableDatas = await backupDescribeTable.call(queryInterface, tableName, options);
      // get foreign key info
      const foreignKeyDatas = (await queryInterface.sequelize.query(`PRAGMA FOREIGN_KEY_LIST("${tableName}");`))[0];
      foreignKeyDatas.forEach((foreignKeyData) => {
        const tableData = tableDatas[foreignKeyData.from];
        tableData.references = { model: foreignKeyData.table, key: foreignKeyData.to };
        tableData.onDelete = foreignKeyData.on_delete;
        tableData.onUpdate = foreignKeyData.on_update;
      });
      if (tableName === 't_device') {
        tableDatas.name.unique = true;
        tableDatas.selector.unique = true;
        tableDatas.external_id.unique = true;
      }
      return tableDatas;
    };
    await queryInterface.changeColumn('t_device', 'name', {
      allowNull: false,
      type: Sequelize.STRING,
      unique: true,
    });
    queryInterface.describeTable = backupDescribeTable;
  },
};

pierre-gilles · Septembre 30, 2019, 12:19

Bon, c’est pas aussi simple que ça… C’est même pire que pensais ^^

Avec le “ON DELETE CASCADE”, quand on migre la table t_device (suppression + recréation), ça vire toutes les entrées de la table t_device_feature et en conséquence de t_device_feature_state… Pas simple!

La seule solution que je vois à notre problème c’est soit:

On fait du méga custom spécial SQLite pour nos migrations… En écrivant nous même en SQL les migrations, et on perd l’idée d’un jour laisser les utilisateurs utiliser du MySQL. Ou alors on fait 2 cas: un cas MySQL, et un cas SQlite (possible, mais c’est du boulot).
On part sur l’idée original, vu que c’est une alpha on demande aux utilisateurs de recréer la DB, et on modifie les migrations.

Je pense honnêtement, que vu l’ampleur que ça prend, on a tout intérêt à modifier les migrations originales (vu qu’on est encore “en dev”, le produit est pas sorti) et à se poser la question quand on aura vraiment pas le choix (c’est à dire, si jamais on a vraiment une column à modifier). En sachant que ça n’arrive que très rarement (il faut vraiment qu’il y ait un défaut de conception, comme ici). En général c’est l’ajout de column le plus fréquent, et ça SQlite le permet

AlexTrovato · Septembre 30, 2019, 4:03

On est encore en dev, ou recrée tout, ça permettra également de re-tester les services et de noter les “imperfections”.