Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Некорректное кодирование параметров запроса #138

Open
ChugunovAN opened this issue Apr 5, 2024 · 13 comments · May be fixed by #139
Open

Некорректное кодирование параметров запроса #138

ChugunovAN opened this issue Apr 5, 2024 · 13 comments · May be fixed by #139

Comments

@ChugunovAN
Copy link

ChugunovAN commented Apr 5, 2024

В функции КодироватьПараметрыЗапроса происходит неявное приведение к типу Строка значения параметра в вызове КодироватьСтроку
ЗначениеПараметра = КодироватьСтроку(Значение, СпособКодированияСтроки.КодировкаURL);

  1. Тип Булево: Истина, Ложь -> "Да", "Нет" (в кодировке URL) вместо "true", "false"
  2. Тип Число: 123456 -> "123 456" (Символы.НПП в кодировке URL) вместо "123456"
  3. Тип Дата: '20240131231012' -> "31.01.2024 23:10:12" (Символ : в кодировке URL) вместо "2024-01-31T23:10:12"

В случае кодирования параметров запроса для тела запроса Content-type="multipart/form-data" не требует кодировки URL, что касается и типа Строка, и других типов.

@leemuar
Copy link
Collaborator

leemuar commented Apr 10, 2024

Спасибо за PR! Но я не уверен, что готов его принять в текущей реализации

Автоматическая конвертация в строку зависит от текущей локали операционной системы, и полагаться на нее не стоит.
Я пока не уверен, что библиотека должна давать какие-то гарантии на подобное преобразование. Может булево должно быть 0 и 1? Или дата должна быть в unix time? Для каждой системы свои правила. Сейчас кажется, что решать такие вопросы - это не ответственность библиотеки

@ChugunovAN
Copy link
Author

Автоматическая конвертация в строку зависит от текущей локали операционной системы, и полагаться на нее не стоит. Я пока не уверен, что библиотека должна давать какие-то гарантии на подобное преобразование. Может булево должно быть 0 и 1? Или дата должна быть в unix time? Для каждой системы свои правила. Сейчас кажется, что решать такие вопросы - это не ответственность библиотеки

Из этой ситуации существуют два выхода:

  1. Либо обязать в передаваемых параметрах использовать только значения типа строка и возложить ответственность за корректную интерпретацию этих значений на вызывающую сторону.
  2. Либо реализовать необходимое преобразование в самом методе. Что уже очень лаконично сделал @alexandr-yang. За что ему огромное спасибо!

@alexandr-yang я в комите оставил маленькое замечание, при исправлении которого поведение станет максимально корректным и для формата multipart/form-data. Заранее буду признателен за любой ответ на этот комментарий.

@leemuar
Copy link
Collaborator

leemuar commented Apr 10, 2024

Из этой ситуации существуют два выхода:

Есть еще третий - оставить конвертацию по стандартной для платформы схеме: в соответствии с локалью операционной системы. т.е. так как работает сейчас.

Я с уважением и благодарностью отношусь к труду всех кто присылает PR, но необходимость такой конвертации самой библиотекой сейчас не достаточно аргументирована. Для разных случаев нужны разные значения, форсировать библиотекой какие-то одни я не считаю коректным. Стандартная конвертация с учетом локали - достаточно привычное поведение платформы, что передать в качестве значения - ответственность программиста.

Я приглашаю к обсуждению и аргументации

@alexandr-yang
Copy link
Contributor

XMLСтрока преобразует в строку четко, без привязки к локали. Поведение будет всегда предсказуемое и на мой взгляд очень логичное.
image

Если нужно как-то "хитро" - тут уже нужно самому преобразовать и передать в параметры строку

@alexandr-yang
Copy link
Contributor

я в комите оставил маленькое замечание

Как по мне, там логика немного сломана. Метод ПодготовитьТелоЗапроса используется не по назначению.

@leemuar
Copy link
Collaborator

leemuar commented Apr 10, 2024

XMLСтрока преобразует в строку четко, без привязки к локали. Поведение будет всегда предсказуемое и на мой взгляд очень логичное.

оно логичное только в рамках одной "предметной" области. В другой логично будет другое, я приводил выше пример с 0 и 1 как булево и unixtime. В JSON, который ближе к web, например, формат даты не стандартизирован, хотя и часто используется unixtime.

Я не могу не отметить изящество в краткости преобразования с помощью XMLСтрока, но само по себе это не аргумент в пользу такого преобразования

P.S. Есть еще другой контраргумент - на текущее поведение кто-то мог завязаться, и таким изменением мы можем сломать людям код. Да, это не документированный сайд-эффект, но все же.

@alexandr-yang
Copy link
Contributor

Что-то сломаться может, тут согласен. В остальном нет. Преобразование булева к числу - это скорее всего костыль, как со стороны сервера, так и со стороны клиента. В вебе чаще всего отправляют true и false. Но это все холивар.

Другой вариант, можно значение пропустить через ОбъектВJson, будет по сути то же самое. А кастомные настройки можно передать через ПараметрыПреобразования (как это сделать красиво - тоже вопрос). Вариант будет более гибкий, но проблему с совместимостью он не решит.

@leemuar
Copy link
Collaborator

leemuar commented Apr 10, 2024

Преобразование булева к числу - это скорее всего костыль, как со стороны сервера, так и со стороны клиента. В вебе чаще всего отправляют true и false.

Не холивар, нормальное обсуждение. Повторюсь, у меня нет цели отказать в PR, цель - обсудить и понять насколько это изменение приемлемо.

В вебе часто применяют и числа: redirect=1. Посмотрите, например, https://www.ibm.com/docs/en/control-desk/7.6.1.2?topic=api-rest-query-parameters Вполне четко написано: "1 для Boolean true, 0 - для false"

true и false часто появляются там где JS фреймворки, потому что такое им проще парсить в объекты JS. Но стандарта на это нет, поэтому я считаю форсировать какое-то соглашение здесь не стоит, это не обязанность библиотеки. И типовое поведение библиотеки здесь - использовать стандартную для платформы сериализацию - вполне нормально.

@alexandr-yang
Copy link
Contributor

Да я тоже не настаиваю в мерже, просто предлагаю решения на основе своего опыта. В общем, решайте. Если нужно будет сделать правки - мне не сложно)

@ChugunovAN
Copy link
Author

Выражу мнение как разработчик, использующий библиотеку: это изменение улучшит лаконичность используемых методов и для разработчика 1С такое приведение значений параметров будет ожидаемым. И можно сказать точно, что Истина/Ложь в виде строки URLEncoded ни в каких кейсах не нужны. Как и Символы.НПП как разделитель триад в числе. С приведением дат к формату yyyy-mm-dd тоже гораздо более ожидаемо, чем формат определяемый локалью.
В особых случаях же (булево 1 или 0, unixtime и т.д.) от разработчика все равно понадобится разобраться с форматом требуемым для конкретного http-сервиса. Что не ухудшит понятность подготовки параметров.

@PLebedevV
Copy link
Contributor

можно сказать точно, что Истина/Ложь в виде строки URLEncoded ни в каких кейсах не нужны. Как и Символы.НПП как разделитель триад в числе. С приведением дат к формату yyyy-mm-dd тоже гораздо более ожидаемо, чем формат определяемый локалью.

полностью согласен

@zeegin
Copy link
Contributor

zeegin commented Apr 23, 2024

Кажется что исправление пользы приносит больше чес вреда.

То что коннектор не дает гарантий на автосериализацию в нужном разработчику формате так это и раньше так было.

То что коннектор станет давать лучшую автоконвертацию чем была факт. А кому надо те как раньше доконвертят вручную.

Важно что меняется обратная совместимость а значит исправление требует поднятие мажорной версии и отражения в BREAKING CHANGE к выпуску.

@alexandr-yang
Copy link
Contributor

Наткнулся на просторах гитхаба, связано с темой обсуждения
huxuxuya/KafkaConfluentRESTProxyAdapter1C#2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants