Баг в SysDictIndex - DAX: Программирование

dech · 15.02.2019, 07:07

Актуальная версия аксапты: AX2012 R3, модификация совместима с более ранними версиями.
Еще в прошлом году нашел баг неверного составления SQL-запроса для поиска записей дубликатов, которые присутствуют в индексе.
Функция вызывается в контекстном меню по конкретному индексу.
Все бы хорошо, но в более ранних версиях, например в АХ4 она в принципе не работает.
Во-первых из-за того, что вызывается на клиенте.
Во-вторых, абсолютно неверно составляется запрос. Функцию невозможно было вызвать ни при каких условиях. Непонятно, как вообще она прошла в production.
В-третьих, хоть она и была исправлена в 2012, однако с некоторыми отключенными полями запрос все-равно формировался с ошибками.
В-четвертых, хотелось бы поменять порядок на более удобный - по убыванию количества дубликатов.

X++:

void showDuplicates()
{
    // > inserted by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    #define.DataAreaNo(2)
    // < inserted by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    tableId         tableId         = this.tableid();
    DictTable       dictTable       = new DictTable(tableId);
    boolean         dataPrCompany   = dictTable.dataPrCompany();
    Counter         numberOfFields  = this.numberOfFields();
    Counter         i;
    str             stmtStr;
    str             resultLineStr;
    str             resultField;
    str             resultField1;
    UserConnection  con             = new UserConnection();
    Statement       stmt            = con.createStatement();
    ResultSet       resultSet;
    boolean         anyDuplicates   = false;
    DictField       dField;
    str             tableNameWithSchema;
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    Map             fields          = new Map(Types::Integer, Types::String);
    MapEnumerator   enumerator;
    Counter         fieldsCount     = #DataAreaNo;
    str             fieldList;
    str             orderList;
    int             key;
    str             fieldName;
    // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    //container       fields;
    //int             enabledFields[];
    //Counter         enabledFieldCounter;
    //SqlStatementExecutePermission ssep;
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    ;
    if (dataPrCompany)
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    {
        fields.insert(fieldsCount, dictTable.fieldName(fieldnum(Common, DataAreaId), DbBackend::Sql));
        fieldsCount++;
    }
    // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    //    fields += dictTable.fieldName(fieldnum(Common,DataAreaId),DbBackend::Sql);
    //
    //enabledFieldCounter = 0;
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates

    for (i = 1; i <= numberOfFields; i++)
    {
        dField = new DictField(tableId, this.field(i));
        // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        if (dField.isSql() && dfield.id() != fieldnum(Common, DataAreaId))
        {
            fields.insert(fieldsCount, dictTable.fieldName(this.field(i), DbBackend::Sql));
            fieldsCount++;
        }
        // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        //if( dField.isSql() == false )
        //    continue;
        //++enabledFieldCounter;
        //enabledFields[enabledFieldCounter] = i;
        // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    }
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    stmtStr = 'select count(*), ';
    enumerator = fields.getEnumerator();
    while (enumerator.moveNext())
    {
        if (any2int(enumerator.currentKey()) != #DataAreaNo)
        {
            fieldList += ', ';
            orderList += ', ';
        }
        fieldList += any2str(enumerator.currentValue());
        orderList += int2str(any2int(enumerator.currentKey()));
    }

    stmtStr += fieldList;
    // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    //for (i = 1; i <= enabledFieldCounter; i++)
    //{
    //    fields += dictTable.fieldName(this.field(enabledFields[i]), DbBackend::Sql);
    //}
    //
    //stmtStr = 'select count(*)';
    //for (i = 1; i <= enabledFieldCounter; i++)
    //{
    //    stmtStr += ', ' + dictTable.fieldName(this.field(enabledFields[i]),DbBackend::Sql,0,FieldNameGenerationMode::FieldListGroupBy);
    //}
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates

    tableNameWithSchema = xSession::getDbSchema();
    if (tableNameWithSchema != '')
        tableNameWithSchema += '.'+ dictTable.name(DbBackend::Sql);
    else
        tableNameWithSchema = dictTable.name(DbBackend::Sql);

    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    stmtStr += strfmt(' from %1 group by %2', tableNameWithSchema, fieldList);
    // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    //stmtStr += ' from ' + tableNameWithSchema;
    //stmtStr += ' group by ';
    //for (i = 1; i <= enabledFieldCounter; i++)
    //{
    //    if (i > 1)
    //        stmtStr += ', ';
    //    stmtStr += dictTable.fieldName(this.field(enabledFields[i]),DbBackend::Sql,0,FieldNameGenerationMode::GroupByFieldList);
    //}
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates

    stmtStr += ' having count(*) > 1';
    if (numberOfFields > 0)
    {
        // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        stmtStr += ' order by 1 desc, ';
        stmtStr += orderList;
        // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        //stmtStr += ' order by ';
        //for (i = 1; i <= enabledFieldCounter; i++)
        //{
        //    if (i > 1)
        //        stmtStr += ', ';
        //    stmtStr += int2str(enabledFields[i]+1);
        //}
        // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        stmtStr += ' desc';
    }

    // dangerous API mitigation
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    new SqlStatementExecutePermission(stmtStr).assert();
    // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
    //ssep =  new SqlStatementExecutePermission(stmtStr);
    //ssep.assert();
    // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates

    // BP Deviation Documented
    resultSet = stmt.executeQuery(stmtStr);

    while (resultSet.next())
    {
        resultLineStr = "@SYS283" + strfmt(': %1', resultSet.getString(1));
        // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        enumerator.reset();
        while (enumerator.moveNext())
        {
            key         = any2int(enumerator.currentKey());
            fieldName   = any2str(enumerator.currentValue());
            resultField = strLRTrim(resultSet.getString(key));
            if (dataPrCompany && key == #DataAreaNo)
                resultField1 = resultField;
            else
                resultLineStr += strfmt(", %1: '%2'", fieldName, resultField);
        }
        // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
        //for (i = 1; i <= numberOfFields; i++)
        //{
        //    resultField = strltrim(resultSet.getString(enabledFields[i]+1));
        //    if (i == 1 && dataPrCompany)
        //        resultField1 = resultField;
        //    else
        //        resultLineStr += strfmt(', %1: \'%2\'', conpeek(fields, enabledFields[i]), resultField);
        //}
        // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates

        if (dataPrCompany)
            // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
            setprefix(strfmt("%1: %2", fieldstr(Common, DataAreaId), resultField1));
            // > modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates
            //setprefix(strfmt('%1: %2', conpeek(fields, 1), resultField1));
            // < modified by dech, 26.11.2018, PPO_CHG0031936_InventDimDuplicates

        info(resultLineStr);
        anyDuplicates = true;
    }
    if (!anyDuplicates)
        info("@SYS68671");

}

Владимир Максимов · 15.02.2019, 12:02

Цитата:

Сообщение от dech

Все бы хорошо, но в более ранних версиях, например в АХ4 она в принципе не работает.
Во-первых из-за того, что вызывается на клиенте.
Во-вторых, абсолютно неверно составляется запрос. Функцию невозможно было вызвать ни при каких условиях. Непонятно, как вообще она прошла в production.
В-третьих, хоть она и была исправлена в 2012, однако с некоторыми отключенными полями запрос все-равно формировался с ошибками.

"Axapta не работает" (с)

Если кому интересно, то проблема в поле DataAreaId. Т.е. для таблиц со свойством SaveDataPerCompany = No данная функция прекрасно работала. А вот если поле DataAreaId надо было учитывать, то вот тут и возникали проблемы с конструированием строки запроса Select-SQL

Это поле не добавлялось в список полей. А здесь нужны 3 списка: Select, Group By, Order By. Поэтому или в полученной строке SQL-запроса появлялось "лишняя" запятая или это поле вообще "забывали", что еще хуже, поскольку получали дубли там, где их нет.

Т.е. модификация сильно избыточная. Надо всего лишь добавить if (dataPrCompany) перед/после формированием очередного списка полей с явным добавлением поля DataAreaId в список полей

Вот здесь был разбор этой функции и зачем нужен параметр fieldNameGenerationMode в методе dictTable.fieldname()

Кнопка "Add-ins\Дубликаты" на табличных индексах в АОТ

PS: Но, вообще-то, есть еще поле Partition. Теоретически, его тоже надо добавлять в формируемый запрос

dech · 15.02.2019, 14:45

Цитата:

Сообщение от Владимир Максимов

Если кому интересно, то проблема в поле DataAreaId.

Проблема не только в этом. Есть некоторые отключенные поля, которые не существуют на уровне SQL Server'а. Список ORDER BY формируется последовательно, чем и порождает ошибку. Например в индексе 10 полей, допустим 2-е, 3-е и 5-е по счету поля отключены, в списке полей их будет всего 7, однако в ORDER BY будет нумерация 1, 4, 6, 7, 8, 9, 10 DESC. А откуда у нас 8, 9 и 10, когда в SELECT'е их всего 7? (я игнорирую COUNT(*) для ясности).

Цитата:

Сообщение от Владимир Максимов

А здесь нужны 3 списка: Select, Group By, Order By.

Достаточно двух списков: названия полей и их номера для сортировки.

Цитата:

Сообщение от Владимир Максимов

Т.е. модификация сильно избыточная. Надо всего лишь добавить if (dataPrCompany) перед/после формированием очередного списка полей с явным добавлением поля DataAreaId в список полей

Как следует из вышесказанного, "всего-лишь добавить if" не получится.
Да и если явно добавить DataAreaId, вы уже не избавитесь от группировки по этому полю для общих таблиц, где DataAreaId нет в принципе.
Плюс я заменил алгоритм на использование коллекции Map. Так будет немного эстетичнее, да и удобнее читать код.

Цитата:

Сообщение от Владимир Максимов

Вот здесь был разбор этой функции и зачем нужен параметр fieldNameGenerationMode в методе dictTable.fieldname()

Кнопка "Add-ins\Дубликаты" на табличных индексах в АОТ

Не спорю, но здесь так не докопались до истины. Проблема осталась висеть мертвым грузом еще 10 лет. Возможно из-за того, что эта функция очень редко используется.

Цитата:

Сообщение от Владимир Максимов

PS: Но, вообще-то, есть еще поле Partition. Теоретически, его тоже надо добавлять в формируемый запрос

Да, можно и с Partition поработать... теоретически, но практически это почти нигде не используется.

Владимир Максимов · 15.02.2019, 18:15

Про order by - это да, не сразу заметил. Но там тоже простая модификация. Хотя вызывает недоумение desc только для последнего поля

Map - эстетичнее и удобнее? Ну, вопрос личных предпочтений. С моей точки зрения наоборот. "Утяжеляет" весь код и существенно усложняет его понимание.

Собственно, проще показать, что достаточно сделать

X++:

// \Classes\SysDictIndex\showDuplicates
void showDuplicates()
{
    tableId         tableId         = this.tableid();
    DictTable       dictTable       = new DictTable(tableId);
    boolean         dataPrCompany   = dictTable.dataPrCompany();
    container       fields;
    Counter         numberOfFields  = this.numberOfFields();
    Counter         i;
    str             stmtStr;
    str             resultLineStr;
    str             resultField;
    str             resultField1;
    UserConnection  con             = new UserConnection();
    Statement       stmt            = con.createStatement();
    ResultSet       resultSet;
    boolean         anyDuplicates   = false;
    SqlStatementExecutePermission ssep;
    DictField dField;
    Counter enabledFieldCounter;
    int enabledFields[];
    str tableNameWithSchema;
    ;
    if (dataPrCompany)
        fields += dictTable.fieldName(fieldnum(Common,DataAreaId),DbBackend::Sql);

    enabledFieldCounter = 0;
    for (i = 1; i <= numberOfFields; i++)
    {
        dField = new DictField(tableId,this.field(i));
        if( dField.isSql() == false )
            continue;
        ++enabledFieldCounter;
        enabledFields[enabledFieldCounter] = i;
    }
    for (i = 1; i <= enabledFieldCounter; i++)
    {
        fields += dictTable.fieldName(this.field(enabledFields[i]), DbBackend::Sql);
    }

    stmtStr = 'select count(*)';
    //+ vmaksimo 15.02.2019
    if (dataPrCompany)
    {
        stmtStr += ', ' + dictTable.fieldName(fieldnum(Common,DataAreaId), DbBackend::Sql, 0, FieldNameGenerationMode::FieldListGroupBy);
    }
    //- vmaksimo 15.02.2019
    
    for (i = 1; i <= enabledFieldCounter; i++)
    {
        stmtStr += ', ' + dictTable.fieldName(this.field(enabledFields[i]), DbBackend::Sql, 0, FieldNameGenerationMode::FieldListGroupBy);
    }
    tableNameWithSchema = xSession::getDbSchema();
    if (tableNameWithSchema != '')
        tableNameWithSchema += '.'+ dictTable.name(DbBackend::Sql);
    else
        tableNameWithSchema = dictTable.name(DbBackend::Sql);


    stmtStr += ' from ' + tableNameWithSchema;
    stmtStr += ' group by ';
    //+ vmaksimo 15.02.2019
    if (dataPrCompany)
    {
        stmtStr += dictTable.fieldName(fieldnum(Common,DataAreaId), DbBackend::Sql, 0, FieldNameGenerationMode::GroupByFieldList); 
        if (enabledFieldCounter)
            stmtStr +=  ', ';
    }
    //- vmaksimo 15.02.2019
    for (i = 1; i <= enabledFieldCounter; i++)
    {
        if (i > 1)
            stmtStr += ', ';
        stmtStr += dictTable.fieldName(this.field(enabledFields[i]), DbBackend::Sql, 0, FieldNameGenerationMode::GroupByFieldList);
    }
    stmtStr += ' having count(*) > 1';
    if (numberOfFields > 0)
    {
        stmtStr += ' order by ';
        
        //+ vmaksimo 15.02.2019
        stmtStr += int2str(1) + ' desc, ';
        //- vmaksimo 15.02.2019
        
        for (i = 1; i <= enabledFieldCounter; i++)
        {
            if (i > 1)
                stmtStr += ', ';
            //+ vmaksimo 15.02.2019
            /*
            stmtStr += int2str(enabledFields[i]+1);
            */
            stmtStr += int2str(i+1) + ' desc';
            //- vmaksimo 15.02.2019
        }
        
        //+ vmaksimo 15.02.2019
        if (dataPrCompany)
        {
            if (i > 1)
                stmtStr += ', ';
            stmtStr += int2str(i+1) + ' desc';
        }
        /*
        stmtStr += ' desc';
        */
        //- vmaksimo 15.02.2019
    }

    // dangerous API mitigation
    ssep =  new SqlStatementExecutePermission(stmtStr);
    ssep.assert();
    // BP Deviation Documented
    resultSet = stmt.executeQuery(stmtStr);

    //+ vmaksimo 15.02.2019
    numberOfFields = enabledFieldCounter;
    if (dataPrCompany)
        numberOfFields++;
    //- vmaksimo 15.02.2019
    
    while (resultSet.next())
    {
        resultLineStr = "@SYS283" + strfmt(': %1', resultSet.getString(1));
        for (i = 1; i <= numberOfFields; i++)
        {
            //+ vmaksimo 15.02.2019
            /*
            resultField = strltrim(resultSet.getString(enabledFields[i]+1));
            */
            resultField = strltrim(resultSet.getString(i+1));
            //- vmaksimo 15.02.2019
            if (i == 1 && dataPrCompany)
                resultField1 = resultField;
            else
                //+ vmaksimo 15.02.2019
                /*
                resultLineStr += strfmt(', %1: \'%2\'', conpeek(fields, enabledFields[i]), resultField);
                */
                resultLineStr += strfmt(', %1: \'%2\'', conpeek(fields, i), resultField);
                //- vmaksimo 15.02.2019
        }
        if (dataPrCompany)
            setprefix(strfmt('%1: %2', conpeek(fields, 1), resultField1));
        info(resultLineStr);
        anyDuplicates = true;
    }
    if (!anyDuplicates)
        info("@SYS68671");

}

Очень тяжело читать код, большая часть которого - это комментарии. В таких случаях проще вообще весь исходный код закомментировать и оставить его в конце метода. А свой код написать "чистым". Без комментариев

dech · 18.02.2019, 08:02

Спасибо за интерес. Ваш код тоже отлично работает.

Цитата:

Сообщение от Владимир Максимов

вызывает недоумение desc только для последнего поля

Я бы оставил DESC только для первого поля, для остальных полей ASC предпочтительнее. Просто не убрал прежний код.

Цитата:

Сообщение от Владимир Максимов

Map - эстетичнее и удобнее? Ну, вопрос личных предпочтений. С моей точки зрения наоборот. "Утяжеляет" весь код и существенно усложняет его понимание.

Любая коллекция упрощает понимание кода. Коллекции позволяют работать на более высоком уровне абстракций. Может и утяжеляет в плане производительности, но насколько? Что нам стоит выделить память под несколько объектов? Не встанет же аксапта колом?

Предлагаю пересмотреть ваше отношение к коллекциям.

Цитата:

Сообщение от Владимир Максимов

Очень тяжело читать код, большая часть которого - это комментарии. В таких случаях проще вообще весь исходный код закомментировать и оставить его в конце метода. А свой код написать "чистым". Без комментариев

Можно и так, с появлением TFS комментарии теперь особо и не нужны. Пишу по регламенту разработки.

Владимир Максимов · 18.02.2019, 11:01

Цитата:

Сообщение от dech

Любая коллекция упрощает понимание кода. Коллекции позволяют работать на более высоком уровне абстракций.

Вопрос спорный и не однозначный. Зависит от конкретной задачи и личных предпочтений разработчика

В данной задаче - все наоборот.

Цитата:

Сообщение от dech

Может и утяжеляет в плане производительности, но насколько? Что нам стоит выделить память под несколько объектов? Не встанет же аксапта колом?

Если вопрос "вообще", то вполне может! Объекты map (имею в виду коллекции) требуют большое количество оперативной памяти. При небольшом количестве элементов (как в данной задаче) - это не критично. Но если речь идет хотя бы о нескольких сотнях элементов, то это чудовищные потери производительности и просто гигантские затраты оперативной памяти.

Показательный пример - это классы работы с оборотно-сальдовыми ведомостями в Dax2009. Чудовищные тормоза и простая замена map на временные таблицы дает резкий скачок производительности даже без оптимизации остального кода.

В данной-то задаче это не критично, но, повторюсь, пихать мапы везде, потому что это "упрощает понимание кода" и "более высокий уровень абстракции" - не стоит. Надо понимать, что придется "принести в жертву" производительность.

Цитата:

Сообщение от dech

Предлагаю пересмотреть ваше отношение к коллекциям.

Так я не против. Просто нельзя бездумно лепить их везде, где это теоретически возможно.

Цитата:

Сообщение от dech

Можно и так, с появлением TFS комментарии теперь особо и не нужны. Пишу по регламенту разработки.

Я к тому, что замена на map, по сути, означает полное переписывание кода. Не исправление замеченных ошибок и недоработок, а именно полная замена. И отдельные вкрапления старого кода

Это в принципе другая идеология написания кода.

В принципе, тут вообще не нужны ни контейнеры, ни map. Я не понимаю, почему не сконструировали команду Select-SQL за один проход по списку полей индекса. Ну, что-то вроде такого

X++:

strSelectList += ' count(*)';
fleldNum = 1;

    if (dataPrCompany)
    {
        fleldNum++;
        strSelectList += ', dataAreaId';
        strGroupList += ', dataAreaId';
        strOrderList += ', ' + int2str(fieldnum);
    }

    for (i = 1; i <= numberOfFields; i++)
    {
        if (...)
        {
            fleldNum++;
            strSelectList += ', ' + fieldname;
            strGroupList += ', ' + fieldname;
            strOrderList += ', ' + int2str(fieldnum);
        }
    }


    stmtStr = 'select ' + strSelectList + 
                   'from ... group by ' + strGroupList + 
                   ' order by ' + strOrderList;

(...)

Это не полноценный код, разумеется. Просто, чтобы показать идею. Здесь контейнер или коллекция нужны разве что для вменяемого сообщения в случае наличия дублей. Да и наполнение этого контейнера/мапа делается там же, в основном (и единственном!) цикле

Здесь разработчик явно действовал "по шаблону". Причем взятому откуда-то из другого класса. Вот и получил то, что получил. Тоже, вероятно, думал об "абстракциях"

Но! Повторюсь, это будет уже не исправление существующего кода, а написание нового. В духе личных предпочтений

SRF · 19.02.2019, 13:03

Дела давно минувших дней

Кнопка "Add-ins\Дубликаты" на табличных индексах в АОТ

SRF · 19.02.2019, 13:16

Цитата:

Сообщение от Владимир Максимов

В принципе, тут вообще не нужны ни контейнеры, ни map. Я не понимаю, почему не сконструировали команду Select-SQL за один проход по списку полей индекса. Ну, что-то вроде такого

Видимо потому, что использовались разные СУБД, не только SQL для более младших версий и поэтому запрос генерировался не прямой, с учетом используемой СУБД (конечно их было не 10, а гораздо меньше, но могли влиять и какие то параметры АОСа), ну а от версии к версии, как правило, MS рефакторингом\оптимизацией кода не занимается, только правит с учетом специфики новой

Zabr · 19.02.2019, 16:55

Цитата:

Сообщение от SRF

Дела давно минувших дней

Оффтоп: Сергей, Ваша ссылка в подписи http://itmagnet.ru/ ведет в никуда.

Похожие темы
Тема	Автор	Раздел	Ответов	Посл. сообщение
Баг или фича - неожиданное поведение метода xRecord.data()	Pandasama	DAX: Программирование	3	21.03.2016 13:01
AX 2012 Пересчет единиц измерения - баг или фича?	Kabardian	DAX: Функционал	3	25.02.2014 11:00
Баг или не баг?	Murlin	DAX: Программирование	53	14.09.2013 18:52
Баг при печати налогового кода	petr	DAX: Программирование	0	25.03.2009 16:33
баг в 2.5. Будьте осторожнее с символом "_" подчеркивание	levsha	DAX: Программирование	5	07.12.2004 12:26