metaclass: (Default)
metaclass ([personal profile] metaclass) wrote2012-10-10 10:39 am
Entry tags:

О проверке входных параметров, принятой в Java

Есть такой оккультный стек-трейс: https://gist.github.com/def1fa8666a8ac83130a.
Причина в том, что в деконструктор массивов кложури вида (let [sql & params] some-array] передан пустой массив и строка sql в таком случае становится null и оный null передается в jdbc драйвер.
Строка на которой валится NPE, выглядит следующим образом:
protected boolean checkForEscapes(String sql) {
sql = sql.toLowerCase();

А вопрос, вообще говоря, в следующем: почему никто не проверяет параметр на валидность хотя бы в одной точке входа в одной из трех либ (clojure.java.jdbc, apache dbcp или jaybird)?

Выглядит так, как будто нормальную проверку параметров на валидность на границах API в жабе делать не принято.

PS: [livejournal.com profile] artureg упорно убеждает меня, что "пусть валится", потому что проверка должна быть "в другом слое". Судя по всему, все разработчики думают, что обработка ошибок должна быть в другом слое :)

[identity profile] artureg.livejournal.com 2012-10-10 07:49 am (UTC)(link)
судя по именам это дао леер, обычно валидация и обработка ошибок происходит выше.
develop7: (dero)

[personal profile] develop7 2012-10-10 07:50 am (UTC)(link)
кстати ребе М настолько суров, что гитхаб падает при попытке показать его профайл

[identity profile] metaclass.livejournal.com 2012-10-10 07:54 am (UTC)(link)
О блин, точно падает с 500
И что ему там не нравится, интересно.

[identity profile] levgem.livejournal.com 2012-10-10 09:18 am (UTC)(link)
отсутствие проверки данных на границе API

[identity profile] bydl0coder.livejournal.com 2012-10-11 01:45 am (UTC)(link)
!!!

[identity profile] bydl0coder.livejournal.com 2012-10-11 08:03 am (UTC)(link)
!!!

[identity profile] artureg.livejournal.com 2012-10-10 07:50 am (UTC)(link)
ксти, а зачем переводить запрос? в нижний регистр?

[identity profile] metaclass.livejournal.com 2012-10-10 07:53 am (UTC)(link)
Проверка на определенные подстроки: https://gist.github.com/3863870

[identity profile] artureg.livejournal.com 2012-10-10 07:55 am (UTC)(link)
тут точно не нужна проверка, так как:
1. если бы sql был нул оно бы упало выше
2. если оно нул надо кидать нпе :)

[identity profile] metaclass.livejournal.com 2012-10-10 08:02 am (UTC)(link)
Этот метод извне либы не вызывается, да. Но они ж и на паблик методах ничего не проверяют :)

[identity profile] jdevelop.livejournal.com 2012-10-10 08:05 am (UTC)(link)
вы знаете, раньше для меня стандартом ебанутости стектрейса был стектрейс спрингового приложения или какого-нибудь уёбложика

теперь это определенно кложура.

[identity profile] metaclass.livejournal.com 2012-10-10 08:14 am (UTC)(link)
В F# та же срань - doInvoke.Invoke$.$Invoker_random$shit$here_ и так на 100 строк)

[identity profile] metaclass.livejournal.com 2012-10-10 08:14 am (UTC)(link)
Покажи аналогичный скальный трейс?

[identity profile] dr-hyder.livejournal.com 2012-10-10 08:16 am (UTC)(link)
У груви обычно даже длиннее. Как звучит то!

[identity profile] zmila.livejournal.com 2012-10-10 08:31 am (UTC)(link)
http://farm6.static.flickr.com/5234/5884828092_7351ae6115.jpg

"The stack trace doesn’t fit in that large monitor in portrait mode. Guys, stop breaking your programs into many functions. Put all your code in one function and your stack traces will be shorter."
:)

[identity profile] thedeemon.livejournal.com 2012-10-10 08:47 am (UTC)(link)
Так вот зачем нужно много длинных мониторов! :)

[identity profile] blackyblack.livejournal.com 2012-10-10 09:33 am (UTC)(link)
Для длинных мониторов нужны горизонтальные логи!

[identity profile] zmila.livejournal.com 2012-10-10 12:10 pm (UTC)(link)
да! адёшь логи-online-неры! :)

[identity profile] kiryl.livejournal.com 2012-10-10 08:52 am (UTC)(link)
Как хорошо, что в ядре стек всего 8k и такой пиздец туда просто не влезет. :)

[identity profile] metaclass.livejournal.com 2012-10-10 09:04 am (UTC)(link)
8k, 4(или 8 байт на адрес возврата), стекфреймы делаем или нет?
в пределе можно 2048 строк в стеке сделать, по идее :)

[identity profile] kiryl.livejournal.com 2012-10-10 09:33 am (UTC)(link)
На x86-64 минимальный stackframe, насколько я помню, 16 байт (код возврата + предыдущее значения rbp). Таким образом 512 вырожденых функций.

Реальные функции имеют переменные на стеке. -fstack-protector, наверно, тоже какой-то оверхед на стеке имеет. Реально 40-50 вложенных функций максимум, но и за такое нужно руки отрывать.

[identity profile] volodymir-k.livejournal.com 2012-10-10 08:15 am (UTC)(link)
Вопрос о месте проверки и способе проявления ошибки непрост и может иметь разные варианты ответа.
По идее, вполне возможно задокументировать NPE как валидную ошибку в методе (@throws NullPointerException if ... are null) и это ВАША проблема, почему вы не прочитали жавадок.

Все эти тривиальные проверки тратят циклы и от них желательно избевляться на верхних уровнях. Для этого в яве экспериментально например сделали @NotNull / @Nullable аннотации и лёгкий inference. Естественно кложа не даёт такого. Что сказать, ну такой язык. Я посмотрел -- неадекват.

[identity profile] zmila.livejournal.com 2012-10-10 08:24 am (UTC)(link)
в самом деле, а ты почему не проверил на nil? :)

и да, в жабе это не принято. обычно клиент проверяет, что он отдаёт либе. а та просто использует, и если что не так, кидается эксцепшенами.

[identity profile] metaclass.livejournal.com 2012-10-10 08:41 am (UTC)(link)
Мой код вообще nil не содержит, а содержит сознательно помещенный туда пустой массив в качестве параметра для clojure.java.jdbc/with-query-results

[identity profile] zmila.livejournal.com 2012-10-10 12:18 pm (UTC)(link)
так у тебя:
(let [[sql & params] some-array] 
  (call-java-shit sql ...) )
или
(let [[sql & params] some-array] 
  (when sql      
    (call-java-shit sql ...) ) )
?
:)


[identity profile] metaclass.livejournal.com 2012-10-10 01:00 pm (UTC)(link)
Первое, но не у меня, а в либе.

[identity profile] max630.livejournal.com 2012-10-10 08:46 am (UTC)(link)
А не похуй ли, в конце концов? Если код уже уехал от места где вместо null должен был быть не null, всё равно ошибку трудно искать.

И да, почему таки там null вместо пустой строки?

[identity profile] metaclass.livejournal.com 2012-10-10 08:51 am (UTC)(link)
деконструктор массивов:
(let [[a b c d] [1 2 3]] [a b c d])
;[1 2 3 nil]

(let [[a b c d] []] [a b c d])
;[nil nil nil nil]

Если массив меньше, чем образец, которым его деконструируют, то соответствующие имена биндятся c nil.
Я передал пустой массив, который в итоге попал в такой деконструктор, первым параметром которого идет строка sql-запроса.

[identity profile] max630.livejournal.com 2012-10-10 09:40 am (UTC)(link)
А потом этот nil передаятся как аргумент в жабную функцию, которая хочет String? И рефлектор радостно подставляет null?

Долбаная динамика. И ведь наверняка найдётся 100500 случаев когда это именно то что надо.

[identity profile] metaclass.livejournal.com 2012-10-10 09:48 am (UTC)(link)
Так null это допустимое значение для String и вообще любых объектов.
Тема null в строках в жабе и дотнете - это отдельная стремная печаль, там строка - это одновременно и ссылка и значение - она иммутабельна, как значение, но является объектом-ссылкой :)

[identity profile] bydl0coder.livejournal.com 2012-10-10 11:53 pm (UTC)(link)
Ну значит деструктор неправ (о пользе паттерн-матчинга first::second::rest, кстати).

[identity profile] metaclass.livejournal.com 2012-10-11 04:58 am (UTC)(link)
Деструктор прав. "Здесь так принято".

[identity profile] andrew kondratovich (from livejournal.com) 2012-10-10 09:09 am (UTC)(link)
Имхо из апи могут сыпаться всякие SuperLibrarySomeException. Но вот не NPE...

[identity profile] golikov konstantine (from livejournal.com) 2012-10-10 03:43 pm (UTC)(link)
допустим, у тебя нет сорсов, тебе из недр библиотечки прилетает NPE или еще чего, и стэктрейс тут не всегда поможет

как ты будешь чинить?
Edited 2012-10-10 15:44 (UTC)

[identity profile] raydac.livejournal.com 2012-10-10 10:03 am (UTC)(link)
вот в одной крупной компании Java программист тоже подумал что кто то на другом уровне проверит, как результат 5000 девайсов на перепрошивку (проданных девайсов причем)
Edited 2012-10-10 10:04 (UTC)

[identity profile] chumpa.livejournal.com 2012-10-10 10:46 am (UTC)(link)
ассерты же! помнится, был приятно удивлён когда включил ассерты и в некоторых хороших библиотеках оно начало срабатывать на нехороших вызовах из моего кода.

[identity profile] juan-gandhi.livejournal.com 2012-10-10 08:17 pm (UTC)(link)
Довольно любопытно, используя бестиповой язык, призывать к массовым проверкам.

[identity profile] bydl0coder.livejournal.com 2012-10-10 11:50 pm (UTC)(link)
+++

[identity profile] metaclass.livejournal.com 2012-10-11 04:56 am (UTC)(link)
Я бы использовал F# или Scala, но мои задачи на них получаются нечитабельными простынями из ASCII арта :)

[identity profile] vinslivins.livejournal.com 2012-11-10 12:00 am (UTC)(link)
гм. а почему это взаимосвязанные вещи?
что мешает мне таскать рядом с каждым значением лямбду с валидацией?

[identity profile] bydl0coder.livejournal.com 2012-10-10 11:50 pm (UTC)(link)
Странный этот ваш артурег. "Пусть валится" это из руби, там принято на любые неправильные параметры реагировать NPE с километровым стектрейсом. В сишарпе я аргументы проверял почти в каждом публичном методе. Кстати, дико вот это вольное обращение с параметрами в руби и джс не нравится, в сишарпе чуть что не так - иди на хуй с ArgumentException, а в динамических либо попытаются таки понять, что ты имел в виду, либо свалятся в дебрях. И то и другое мешает отладке.
Edited 2012-10-11 00:07 (UTC)

[identity profile] metaclass.livejournal.com 2012-10-11 04:57 am (UTC)(link)
Ну, я ж с C# и пришел - там везде проверки и ArgumentException, а тут в жабах бардак какой-то. "Циклы экономят". Один cmp/jne вызов, ага.

[identity profile] jdevelop.livejournal.com 2012-10-11 04:29 pm (UTC)(link)
ребе, assert и -ea