UGENE-5291 WD can't work with files with ; in name #1588

rasputinkirill · 2024-03-19T00:13:54Z

Устроит такое решение?
Если да дополню тестом и трансляциями.

yalgaer · 2024-03-19T03:03:23Z

Я бы попробовала корректно поддержать файлы с допустимыми символами.
У нас есть код, который объединяет списки имен файлов используя ';' и после восстанавливает оригинальные имена из списков (WorkflowUtils::unpackListOfDatasets).

Нужно сделать 2 модификации в этих методах: энкодить и декодить "плохие" имена файлов по примеру того как это делается в веб: https://doc.qt.io/qt-6/qurl.html#toEncoded.

Можно выбрать любой свой вид энкодинга (энкодить только ';' ), так чтобы при этом максимально сохранялось визуальное представление файла в закодированном виде.

rasputinkirill · 2024-03-19T07:52:53Z

Решение с энкодингом не универсально.
Дело в том, что URLLineEdit и в кастомные поля воркеров пользователь может ввести данные руками, и мы не сможем определить где пользователь имел ввиду точку с запятой в имени файла, а где как разделитель файлов.
Поэтому остается абсолютно такая же ситуация как в текущем решении, тут мы можем гарантированно определить когда пользователь подразумевает несколько файлов, а когда имя с точкой с запятой только в случае с диалогами. А в случае ручного ввода мы не сможем определить и корректно заэнкодить.
Так что все же предлагаем оставить мое решение:

проверять корректность имен файлов в случае ввода через диалог и информировать пользователя
проверять на на этапе валидации схемы входные url (сюда попадут все которые были введены вручную)

yalgaer · 2024-03-19T17:36:27Z

Если такой файл может быть в файловой системе, то UGENE должен уметь его открывать. Давай решать задачу в этом контексте.

rasputinkirill · 2024-03-19T19:05:18Z

Если такой файл может быть в файловой системе, то UGENE должен уметь его открывать. Давай решать задачу в этом контексте.

У меня есть только такой вариант решения:
В WorkflowUtils::validateInputFiles когда полученый на вход список url засплитили в список, валидатор в цикле начинает проверять их наличие на файловой системе. Если файла нету - он пытается конструирует url из текущего элемента, точки с запятой и следующего элемента списка, и снова проверяет наличие файла на файловой системе и так пока не найдет файл или не закончится список.
Если список закончился, а файл не обнаружен пишет ошибку "Не могу найти один из файлов из этого списка: " и выводит список с того элемента на котором застрял.

Если честно мне не сильно нравится эта идея, не нравится этот "элемент угадывания", мне кажется программа так не должна работать. Вариант с информированием пользователя об ограничении мне больше нравится.

Ну или предложи свое решение.

yalgaer · 2024-03-20T01:24:12Z

Решение простое: изучить и выбрать какой вид encoding в данном случае будет наиболее приемлем.

Кодирование/декодирование URL- это именно то что используют как командные шелы в операционных системах, так и строки с параметрами в интернет.

Примеры:

Имя файла может содержать пробел, но в командной строке это разделитель параметров. Как-то это проблема решается как на Windows так и на Linux
Значение параметра в URL может содержать знак =. Но знак = это разделитель ключ/значение для параметров. И тут проблема решена.

Так же и в UGENE это проблема может быть решена во всех местах где мы сериализируем/парсим список имен файлов.

К самой файловой системе при этом обращаться не нужно - список имен файлов должен однозначно кодироваться и декодироваться без внешнего контекста.

Проанализируй и выбери наиболее приемлемый нам вариант решения. Например:

Эскейпить ';' если это часть имени файла
Заключать имя файла в кавычки если в нем есть "плохие" символы и эскейпить кавычки.
Энкодить плохие символы как URL параметры в интернет.
Что-то еще более крутое и интересное.

rasputinkirill · 2024-03-27T03:47:50Z

Попробовал поэкспериментировать с экранированием кавычками, но столкнулся с проблемой.
QFileInfo и QFile в принципе не работают нормально с путями в которых есть ";" и путь юникс-стайл.
Вызовы функций вернут false

    QFileInfo ffff("C:/ugene_issue_files/5291/A;nnota;tio;ns.txt");
    ffff.exists();
    QFile::exists("C:/ugene_issue_files/5291/A;nnota;tio;ns.txt");

На линуксе сам юджин (не WD) даже не открывает файлы по таким путям.

yalgaer · 2024-04-01T02:05:51Z

Попробовал поэкспериментировать с экранированием кавычками, но столкнулся с проблемой. QFileInfo и QFile в принципе не работают нормально с путями в которых есть ";" и путь юникс-стайл. Вызовы функций вернут false
    QFileInfo ffff("C:/ugene_issue_files/5291/A;nnota;tio;ns.txt");
    ffff.exists();
    QFile::exists("C:/ugene_issue_files/5291/A;nnota;tio;ns.txt");    
На линуксе сам юджин (не WD) даже не открывает файлы по таким путям.

Попробуй заескейпить плохие символы. Что если у QT спросить список файлов в каталоге среди которых будет "плохие"?
Мне кажется не может такого быть чтобы на Linux были файлы которые QT вообще не может открыть.

rasputinkirill · 2024-04-01T20:53:32Z

QFileInfo ffff("C:/ugene_issue_files/5291/A\;nnota\;tio\;ns.txt");
ffff.exists();
QFile::exists("C:/ugene_issue_files/5291/A\;nnota\;tio\;ns.txt");

Заэскейпленые не воспринимает.

QDir dir("C:/ugene_issue_files/5291/");
QStringList entries = dir.entryList();

Тут в списке все файлы присутствуют.
На линуксе так же. Причем при сборке ворнинги на \; Unknown escape sequence.

Если что то, скриншот выше это месседж бокса с ошибкой - это я просто через File->Open попробовал открыть файл с проблемным именем. Это я к тому что если делать поддержку точки с запятой в путях, то это надо будет по всему приложению в итоге делать, а не только в дизайнере.

yalgaer · 2024-04-02T02:54:00Z

Qt нормально работает с файлами у которых в имени есть точка с запятой:

В рамках этого бага достаточно исправить описанную проблему - не нужно переделывать весь UGENE. На остальные места можно завести другие баги.

rasputinkirill · 2024-04-09T12:31:35Z

Попробовал сделать с экранированием всей строки заключением в двойные кавычки.
Но столкнулся с проблемой, что в самом формате uwl файла который создается и потом запускается присутствуют кавычки, а если при сериализации файла он встречает двойные кавычки то меняет их на одинарные.
Экранировать пути в одинарными кавычками тоже не выйдет - так как это допустимый в путях файлов.
Так же а принципе при любом типе экранирования, хоть символа, хоть всей строки в кодле придется обрабатывать все места где мы этот файл пытаемся открыть.
А еще тут не видно сразу масштаба необходимых работ, исправляешь одну проблему - появляяются еще места где это не работает.

Пока я все это пытался делать обнаружил что датасеты должны нормально работать с путями с ";" так как там файлы всегда передаются для обработки по одному и неопределенности не возникает.
Поэтому предлагаю такие исправления:

проверить что датасеты корректно обрабатывют проблемные пути и убрать проверку для них
в URLLineEdit при вводе проблемного пути выдавать пользователю предупреждение

yalgaer · 2024-04-10T03:41:51Z

А что именно будет в предупреждении?
Если мы скажем что в результате будет неопределенное поведение - то это нехорошо.

Можно ли при сохранении в UWL экранировать и переводы строк и кавычки по примеру URL encoding?
Это может быть расширением возможностей формата UWL: например так:

files: urls("url1", "url2"...) где url в кавычках уже безопасный (encoded)
?

rasputinkirill · 2024-04-11T08:22:09Z

А что именно будет в предупреждении? Если мы скажем что в результате будет неопределенное поведение - то это нехорошо.

В предупреждении будет написано "путь до файла ';', такие пути не поддерживаются этим элементом, переименуйте и/или положите файл в другую папку.".

Можно ли при сохранении в UWL экранировать и переводы строк и кавычки по примеру URL encoding? Это может быть расширением возможностей формата UWL: например так:

files: urls("url1", "url2"...) где url в кавычках уже безопасный (encoded) ?

Думаю, если делать "по-красоте" то для данных из URLLineEdit обертку как для dataset и показывать пользователю в воркере пути как он их ввел - с ";" в путях, а внутри юджина гонять эти данные в заэнкоженном виде. Так же и в схему писать в заэнкоженном виде, а при загрузке и показе схемы пользователю приводить к "человеческому" виду.

Я бы ограничился с вариантом с проверкой работоспособности датасетов и предупреждением в URLLineEdit. Мне кажется поддержка такого вида имен файлов не стоит уже потраченного времени на эту задачу.

yalgaer · 2024-04-12T04:08:30Z

В качестве временного/быстрого решения можно сделать так, чтобы UGENE вообще не давал записывать/запускать то, в результате чего он не уверен.

Можно ли вместо "предупреждения" сделать блокировку на запуск?

rasputinkirill · 2024-04-12T12:23:43Z

Не совсем понял что имеешь в виду, ведь сейчас так и есть. Пытаемся запустить схему - валидатор говорит что в схеме ошибки тоько они выглядят коряво из-за попытки распаристь путь по кускам разделенными ";", например для пути "c:\ugene_issue_files\5291\A;;nnota;;tio;n_names.txt" будет так:

Схема запущена не будет.

yalgaer · 2024-04-14T00:19:16Z

То есть если мы идем по пути отказа от полноценного фикса, получается что мы не можем отличить ситуацию где ; это символ в имени файла или ; это символ разделитель разных имен.

Если мы не можем различить - не нужно ни warning, ни ошибки - мы должны работать с ; как с разделителем.

Если же мы знаем, что ; каким-то образом пришел в элемент как символ в имени файла - тут мы должны отказать в исполнении/записи этого имени, так как при чтении мы будем трактовать его как разделитель. Правильно?

rasputinkirill · 2024-04-14T02:54:38Z

То есть если мы идем по пути отказа от полноценного фикса, получается что мы не можем отличить ситуацию где ; это символ в имени файла или ; это символ разделитель разных имен.

Если мы не можем различить - не нужно ни warning, ни ошибки - мы должны работать с ; как с разделителем.

Сейчас работает так - во время валидации когда проверяется что все входные пути существуют ; интерпретируется как разделитель. Появляются ошибки валидации и все прерывается, предлагаешь убрать эту проверку в валидаторе схемы?

Если же мы знаем, что ; каким-то образом пришел в элемент как символ в имени файла - тут мы должны отказать в исполнении/записи этого имени, так как при чтении мы будем трактовать его как разделитель. Правильно?

Ну не совсем, я предлагал что если пользователь ввел в URLLineEdit путь с ; то не принимать его ввод и выдавать MessageBox с просьбой переименовать/поменять путь.
При этом так же проверить и отредактировать проверку для путей пришедших из датасетов так как пути из них обрабатываются по одной штуке и ; в них никогда не бывает разделителем.

Так же мне тут еще идея пришла, сделать класс URLLineEdit для мест где ожидается ввод одного пути и для мест где вводится список. Тогда, если из URLLineEdit в котором ожидается ввод одного пути пришел url то не пытаться парсить его как список файлов и тогда будет однозначность насчет данных пришедших из него. А проверку и предупреждение делать только в тех где список вводим.
У меня если честно ощущение, что у нас даже нет (или их буквально единицы) воркеров где мы ожидаем ввод списка файлов.
Тогда получится что в датасетах проблемные пути допустимы, в большей части воркеров допустимы и только в некоторых будет предупреждение при вводе.

yalgaer · 2024-04-15T02:26:06Z

Мне было бы интересно сделать поддержку multiple URLS в UWL таким образом, что не возникает неопределенности.
Но если это перебор - давай сделаем минимум твоим способом. Напиши как тот простой и лучший с твоей точки зрения способ готов для review

This reverts commit 1a2a15a.

rasputinkirill · 2024-04-16T00:41:18Z

Поменял проверку путей для датасетов, так как там всегда единственный url приходит для проверки.
Директории тоже всегда по одной проверяются, поэтому переименовал и поменял метод.
В URLLineEdit добавил отображение предупреждения

src/corelibs/U2Designer/src/WorkflowGUIUtils.h

src/corelibs/U2Designer/src/support/URLLineEdit.cpp

yalgaer · 2024-04-17T01:38:59Z

src/corelibs/U2Lang/src/support/WorkflowUtils.cpp

+    const QStringList urlsList = urls.split(';');
+    for (const QString& url : qAsConst(urlsList)) {
+        CHECK_EXT_CONTINUE(validateInputFile(url, notificationList));
+        res = false;
    }


Есть ли смысл тут продолжать проверять файлы после первой ошибки? Ошибок может быть тысячи?

Тоже думал так сделать, но оставил старое поведение.
Если пользователь получит ошибку на пачку файлов он их пачкой и исправит, а не по одной будет. Можно ввести лимит на количество ошибок, например. IDE так же делают. Что думаешь?

Нужно выдавать только одну ошибку. Если хочешь можешь в ее текст включить первые N файлов

rasputinkirill · 2024-04-17T05:30:52Z

Исправление устраивает, пишу тесты?

yalgaer · 2024-04-19T03:47:39Z

Исправление устраивает, пишу тесты?

Да

src/corelibs/U2Designer/src/support/URLLineEdit.cpp

src/corelibs/U2Lang/src/support/WorkflowUtils.cpp

yalgaer

Меняю статус requested review -> request changes. Причина: часть комментариев не исправлена

yalgaer

Ну нужно говорить в сообщении об ошибке о символе ;

Вероятность имено этой исчезающе мала по сравнению с обычной: файл не найден по каким-то другим причинам. В этом случае мы только запутаем пользователя своей рекомендацией. Можешь добавить это в доку если хочешь и сослаться из ошибки на доку.

Достаточно показать пользователю имя файла как мы его понимаем - дальше он поймет сам что не так

rasputinkirill · 2024-05-20T09:21:17Z

Хорошо, поменял в случае ручного ввода. Если пользователь выбирал через гуи оставил текущее сообщение - тут вполне однозначно что выбран файл с плохим путем\именем.

yalgaer

Тест должен быть без workarounds и извинений: если иначе не работает, надо исправлять или просто писать тест иначе (так как текущие проблемы в тесте не относятся к тестируемуму исправлению)

Если мы оставили 2 варианта поведения: c и без ручнного ввода: можно ли протестировать оба варианта?

rasputinkirill · 2024-05-21T15:52:51Z

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

yalgaer · 2024-05-22T12:39:52Z

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

Перекликивание говорит о баге в WD - либо элемент в фокусе и на него не нужно кликать чтобы увидеть его детали, либо не в фокусе - тогда достаточно кликнуть только на него.

Кликать на другой элемент и потом возвращаться к текущему - это workaround для бага.

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

Не вижу где тестируется текст нового сообщения об ошибке

rasputinkirill · 2024-05-22T13:39:33Z

Так устроит?

yalgaer · 2024-05-23T19:32:53Z

Замечаний нет, жду зеленые тесты

rasputinkirill · 2024-05-29T04:20:47Z

На маке несвязанный тест упал, считаем что успех?

UGENE-5291 WD can't work with files with ; in name

1a2a15a

rasputinkirill requested a review from a team as a code owner March 19, 2024 00:13

rasputinkirill requested a review from yalgaer March 19, 2024 00:13

Kirill Rasputin added 4 commits April 16, 2024 02:03

Merge remote-tracking branch 'remotes/origin/master' into UGENE-5291

21ef29c

Revert "UGENE-5291 WD can't work with files with ; in name"

902ea7d

This reverts commit 1a2a15a.

UGENE-5291

d980cf6

UGENE-5291

03ffcbb

yalgaer reviewed Apr 17, 2024

View reviewed changes

rasputinkirill requested a review from yalgaer April 17, 2024 05:31

UGENE-5291

b6deec6

yalgaer reviewed May 5, 2024

View reviewed changes

src/corelibs/U2Designer/src/support/URLLineEdit.cpp Outdated Show resolved Hide resolved

rasputinkirill requested a review from yalgaer May 5, 2024 09:21

Kirill Rasputin added 3 commits May 5, 2024 17:18

xml text fix

b8d9170

xml test

cab85d8

xml test update

fe595bf

yalgaer reviewed May 7, 2024

View reviewed changes

src/corelibs/U2Designer/src/support/URLLineEdit.cpp Outdated Show resolved Hide resolved

yalgaer reviewed May 7, 2024

View reviewed changes

src/corelibs/U2Lang/src/support/WorkflowUtils.cpp Show resolved Hide resolved

rasputinkirill requested a review from yalgaer May 7, 2024 14:36

yalgaer requested changes May 7, 2024

View reviewed changes

Kirill Rasputin added 3 commits May 13, 2024 17:46

UGENE-5291

185ca69

Merge remote-tracking branch 'remotes/origin/master' into UGENE-5291

594c97d

UGENE-5291

d462abe

rasputinkirill requested a review from yalgaer May 13, 2024 11:39

Kirill Rasputin added 2 commits May 13, 2024 19:59

warning fix

b6b3a8c

UGENE-5291

3cd7a62

yalgaer requested changes May 15, 2024

View reviewed changes

message updated

8870108

rasputinkirill requested a review from yalgaer May 20, 2024 09:21

yalgaer requested changes May 21, 2024

View reviewed changes

rasputinkirill requested a review from yalgaer May 21, 2024 15:52

UGENE-5291

df10429

Kirill Rasputin added 2 commits May 27, 2024 14:24

test update

4f5fd5e

Merge remote-tracking branch 'remotes/origin/master' into UGENE-5291

a4beea3

yalgaer approved these changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UGENE-5291 WD can't work with files with ; in name #1588

UGENE-5291 WD can't work with files with ; in name #1588

rasputinkirill commented Mar 19, 2024 •

edited

yalgaer commented Mar 19, 2024

rasputinkirill commented Mar 19, 2024

yalgaer commented Mar 19, 2024

rasputinkirill commented Mar 19, 2024

yalgaer commented Mar 20, 2024

rasputinkirill commented Mar 27, 2024

yalgaer commented Apr 1, 2024

rasputinkirill commented Apr 1, 2024

yalgaer commented Apr 2, 2024

rasputinkirill commented Apr 9, 2024

yalgaer commented Apr 10, 2024

rasputinkirill commented Apr 11, 2024

yalgaer commented Apr 12, 2024

rasputinkirill commented Apr 12, 2024 •

edited

yalgaer commented Apr 14, 2024

rasputinkirill commented Apr 14, 2024

yalgaer commented Apr 15, 2024

rasputinkirill commented Apr 16, 2024

yalgaer Apr 17, 2024

rasputinkirill Apr 17, 2024

yalgaer May 7, 2024

rasputinkirill commented Apr 17, 2024

yalgaer commented Apr 19, 2024

yalgaer left a comment

yalgaer left a comment

rasputinkirill commented May 20, 2024

yalgaer left a comment

rasputinkirill commented May 21, 2024 •

edited

yalgaer commented May 22, 2024

rasputinkirill commented May 22, 2024

yalgaer commented May 23, 2024

rasputinkirill commented May 29, 2024

UGENE-5291 WD can't work with files with ; in name #1588

Are you sure you want to change the base?

UGENE-5291 WD can't work with files with ; in name #1588

Conversation

rasputinkirill commented Mar 19, 2024 • edited

yalgaer commented Mar 19, 2024

rasputinkirill commented Mar 19, 2024

yalgaer commented Mar 19, 2024

rasputinkirill commented Mar 19, 2024

yalgaer commented Mar 20, 2024

rasputinkirill commented Mar 27, 2024

yalgaer commented Apr 1, 2024

rasputinkirill commented Apr 1, 2024

yalgaer commented Apr 2, 2024

rasputinkirill commented Apr 9, 2024

yalgaer commented Apr 10, 2024

rasputinkirill commented Apr 11, 2024

yalgaer commented Apr 12, 2024

rasputinkirill commented Apr 12, 2024 • edited

yalgaer commented Apr 14, 2024

rasputinkirill commented Apr 14, 2024

yalgaer commented Apr 15, 2024

rasputinkirill commented Apr 16, 2024

yalgaer Apr 17, 2024

Choose a reason for hiding this comment

rasputinkirill Apr 17, 2024

Choose a reason for hiding this comment

yalgaer May 7, 2024

Choose a reason for hiding this comment

rasputinkirill commented Apr 17, 2024

yalgaer commented Apr 19, 2024

yalgaer left a comment

Choose a reason for hiding this comment

yalgaer left a comment

Choose a reason for hiding this comment

rasputinkirill commented May 20, 2024

yalgaer left a comment

Choose a reason for hiding this comment

rasputinkirill commented May 21, 2024 • edited

yalgaer commented May 22, 2024

rasputinkirill commented May 22, 2024

yalgaer commented May 23, 2024

rasputinkirill commented May 29, 2024

rasputinkirill commented Mar 19, 2024 •

edited

rasputinkirill commented Apr 12, 2024 •

edited

rasputinkirill commented May 21, 2024 •

edited