search

ugeneunipro / ugene

UGENE is free open-source cross-platform bioinformatics software
http://ugene.net
GNU General Public License v2.0
213 stars 62 forks source link

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

Closed rasputinkirill closed 5 months ago

rasputinkirill commented 7 months ago

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

yalgaer commented 7 months ago

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

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

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

rasputinkirill commented 7 months ago

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

yalgaer commented 7 months ago

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

rasputinkirill commented 7 months ago

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

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

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

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

yalgaer commented 7 months ago

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

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

Примеры:

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

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

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

Проанализируй и выбери наиболее приемлемый нам вариант решения. Например: 1) Эскейпить ';' если это часть имени файла 2) Заключать имя файла в кавычки если в нем есть "плохие" символы и эскейпить кавычки. 3) Энкодить плохие символы как URL параметры в интернет. 4) Что-то еще более крутое и интересное.

rasputinkirill commented 7 months ago

Попробовал поэкспериментировать с экранированием кавычками, но столкнулся с проблемой. 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) даже не открывает файлы по таким путям. image

yalgaer commented 7 months ago

Попробовал поэкспериментировать с экранированием кавычками, но столкнулся с проблемой. 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) даже не открывает файлы по таким путям. image

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

rasputinkirill commented 7 months ago 
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 commented 7 months ago

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

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

rasputinkirill commented 7 months ago

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

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

yalgaer commented 7 months ago

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

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

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

rasputinkirill commented 6 months ago

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

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

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

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

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

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

yalgaer commented 6 months ago

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

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

rasputinkirill commented 6 months ago

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

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

yalgaer commented 6 months ago

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

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

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

rasputinkirill commented 6 months ago

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

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

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

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

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

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

yalgaer commented 6 months ago

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

rasputinkirill commented 6 months ago

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

rasputinkirill commented 6 months ago

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

yalgaer commented 6 months ago

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

Да

rasputinkirill commented 5 months ago

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

rasputinkirill commented 5 months ago

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

yalgaer commented 5 months ago

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

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

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

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

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

rasputinkirill commented 5 months ago

Так устроит?

yalgaer commented 5 months ago

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

rasputinkirill commented 5 months ago

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