feat: Добавить валидацию обработчиков событий#56
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a qualified-method-name parser, updates the delegate factory to use it, extends the run-validator to reflectively validate event handlers are exported procedures with exactly one parameter, and updates/adds tests and fixtures to cover non-exported and invalid handlers and parameter sources. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Validator
participant Parser
participant Module
participant Reflector
Client->>Validator: Initiate run / register handler
Validator->>Validator: ПроверитьОбработчикиСобытий()
Validator->>Parser: Разобрать(КвалифицированноеИмя)
Parser->>Module: Вычислить(квалификатор)
Module-->>Parser: Модуль/Тип или Неопределено
Parser-->>Validator: {Владелец, ИмяМетода}
Validator->>Reflector: ПолучитьТаблицуМетодов(Владелец)
Reflector-->>Validator: ТаблицаМетодов или Исключение
Validator->>Validator: Проверить: экспортность и кол-во параметров == 1
alt Невалидный обработчик
Validator->>Client: Записать сообщение об ошибке
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 41 minutes and 57 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os (1)
340-348: Не скрывайте все исключения при поиске метода без диагностического контекста.Полное подавление исключений маскирует реальные причины (ошибка разбора, проблема рефлексии, и т.д.) и затрудняет отладку. Лучше сохранять причину (лог/техническое сообщение) и возвращать управляемую ошибку валидации.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os` around lines 340 - 348, Блок, который пытаетcя найти метод полностью подавляет все исключения, из-за чего теряются причины ошибок; измените обработчик в методе где используется РазборКвалифицированногоИмениМетода/Разобрать и вызов _Рефлектор.ПолучитьТаблицуМетодов/ТаблицаМетодов.Найти: перехватывайте исключение, записывайте его техническое сообщение в лог (или сохраняйте в поле диагностической информации) и вместо молчаливого возврата Неопределено генерируйте контролируемую валидационную ошибку или возвращайте структуру результата с полем ошибки/диагностики, включающей исходное исключение и контекст (КвалифицированноеИмяМетода, ТипКлассаБенчмарков).src/BenchmarkOneScript/core/common/Классы/РазборКвалифицированногоИмениМетода.os (1)
1-31: Сделайте имя нового типа более предметным, а не техническим.
РазборКвалифицированногоИмениМетодаописывает техническую операцию, а не доменную роль. Лучше выбрать доменное имя (например, вокруг “резолвинга обработчика/владельца метода”), чтобы API читался на уровне предметной области.As per coding guidelines
{src,tests}/**/*.os: "Make new type names, method names, and test file names domain-specific rather than technical".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/BenchmarkOneScript/core/common/Классы/РазборКвалифицированногоИмениМетода.os`:
- Around line 75-85: В функции ПолучитьМодульПоИмени замените опасный вызов
Вычислить(ИмяМодуля) на безопасную проверку и поиск: сначала валидируйте строку
ИмяМодуля по допустимому идентификатору (например, регулярным выражением для
[A-Za-z_][A-Za-z0-9_]*), затем используйте безопасный способ разрешения модуля —
например проверить Тип(ИмяМодуля) или сверять ИмяМодуля со списком/whitelist
известных модулей и только после этого возвращать объект модуля; удалите или
избегайте выполнения произвольных выражений через Вычислить, и сохраните
существующую проверку на Тип("Сценарий") и бросание исключения при
несоответствии.
In `@src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os`:
- Around line 324-334: В текущей проверке нужно гарантировать, что метод —
процедура, а не функция: дополнительно проверить свойство рефлектора
СвойстваМетода.ЭтоФункция и при его истинном значении возвращать Ложь; т.е. в
логике валидатора рядом с существующими проверками (СвойстваМетода.Экспорт и
проверкой ПараметрыМетода) добавить условие, которое отбрасывает методы, у
которых СвойстваМетода.ЭтоФункция = Истина, чтобы допускались только процедуры.
---
Nitpick comments:
In `@src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os`:
- Around line 340-348: Блок, который пытаетcя найти метод полностью подавляет
все исключения, из-за чего теряются причины ошибок; измените обработчик в методе
где используется РазборКвалифицированногоИмениМетода/Разобрать и вызов
_Рефлектор.ПолучитьТаблицуМетодов/ТаблицаМетодов.Найти: перехватывайте
исключение, записывайте его техническое сообщение в лог (или сохраняйте в поле
диагностической информации) и вместо молчаливого возврата Неопределено
генерируйте контролируемую валидационную ошибку или возвращайте структуру
результата с полем ошибки/диагностики, включающей исходное исключение и контекст
(КвалифицированноеИмяМетода, ТипКлассаБенчмарков).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5333e710-b367-4ca3-a413-68fdc637670d
📒 Files selected for processing (5)
src/BenchmarkOneScript/core/common/Классы/РазборКвалифицированногоИмениМетода.ossrc/BenchmarkOneScript/core/common/Классы/ФабрикаДелегата.ossrc/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.ostests/fixtures/benchmarks/Классы/БенчмаркСОбработчикамиСобытий.ostests/ТестыОбработчиковСобытийБенчмарков.os
| Если Не СвойстваМетода.Экспорт Тогда | ||
| Возврат Ложь; | ||
| КонецЕсли; | ||
|
|
||
| ПараметрыМетода = СвойстваМетода.Параметры; | ||
|
|
||
| Если ПараметрыМетода.Количество() <> 1 Тогда | ||
| Возврат Ложь; | ||
| КонецЕсли; | ||
|
|
||
| Возврат Истина; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file around lines 324-334 to understand the context
cat -n "src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os" | sed -n '300,350p'Repository: Stivo182/BenchmarkOneScript
Length of output: 1716
🏁 Script executed:
# Let's also check the full file size to understand its structure
wc -l "src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os"Repository: Stivo182/BenchmarkOneScript
Length of output: 148
🏁 Script executed:
# Search for method type related keywords in the file
rg -n "Тип|Процедура|Функция" "src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os" | head -30Repository: Stivo182/BenchmarkOneScript
Length of output: 1674
🏁 Script executed:
# Look for the method that contains these lines
ast-grep --pattern 'function $NAME() {
$$$
Если Не СвойстваМетода.Экспорт Тогда
$$$
}'Repository: Stivo182/BenchmarkOneScript
Length of output: 53
🏁 Script executed:
# Search for usage of method table properties to see what fields are accessed
rg -n "ТаблицаМетодов|СвойстваМетода\." "src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os"Repository: Stivo182/BenchmarkOneScript
Length of output: 314
🏁 Script executed:
# Search for method type checking patterns in the codebase
rg -i "Процедура|Функция|ТипМетода|МетодТип" "src/" --type os | head -40Repository: Stivo182/BenchmarkOneScript
Length of output: 98
🏁 Script executed:
# Look for other validator files or similar validation patterns
fd "Валидатор|валидатор" "src/" --type fRepository: Stivo182/BenchmarkOneScript
Length of output: 144
🏁 Script executed:
# Search for reflector usage to understand its API
rg "_Рефлектор\." "src/" --type os -A 2 | head -50Repository: Stivo182/BenchmarkOneScript
Length of output: 98
🏁 Script executed:
# Search for reflector usage in all os files
grep -r "_Рефлектор\." "src/" --include="*.os" | head -40Repository: Stivo182/BenchmarkOneScript
Length of output: 1611
🏁 Script executed:
# Search for method property access patterns
grep -r "СвойстваМетода\." "src/" --include="*.os"Repository: Stivo182/BenchmarkOneScript
Length of output: 2719
🏁 Script executed:
# Look for procedure/function type checking in validators or similar code
grep -r "Процедура\|Функция" "src/" --include="*.os" | grep -i "тип\|check\|валид" | head -20Repository: Stivo182/BenchmarkOneScript
Length of output: 247
🏁 Script executed:
# Search for Рефлектор class definition or initialization
grep -r "Рефлектор" "src/" --include="*.os" -B 2 -A 5 | head -60Repository: Stivo182/BenchmarkOneScript
Length of output: 7033
🏁 Script executed:
# Check if there's any documentation or comments about method properties
grep -r "ТипМетода\|МетодТип\|метода\.\(Процедура\|Функция\)" "src/" --include="*.os"Repository: Stivo182/BenchmarkOneScript
Length of output: 53
🏁 Script executed:
# Look for all properties accessed on method objects to find method type indicator
grep -r "СвойстваМетода\.\|СвойстваКонструктора\." "src/" --include="*.os" -o | sort | uniqRepository: Stivo182/BenchmarkOneScript
Length of output: 635
🏁 Script executed:
# Search in config file for method property documentation
grep -r "Параметры\|Возвращаемое\|тип метода\|процедур" "src/BenchmarkOneScript/core/config/Классы/ИзвлекательКонфигурацииБенчмарков.os" -B 3 -A 3 | head -50Repository: Stivo182/BenchmarkOneScript
Length of output: 1749
🏁 Script executed:
# Look for РефлекторОбъекта class definition
find "src/" -name "*.os" -exec grep -l "class РефлекторОбъекта\|Класс РефлекторОбъекта" {} \;Repository: Stivo182/BenchmarkOneScript
Length of output: 53
🏁 Script executed:
# Search for method table structure documentation or usage patterns
grep -r "ПолучитьТаблицуМетодов" "src/" --include="*.os" -B 5 -A 10 | head -100Repository: Stivo182/BenchmarkOneScript
Length of output: 10518
🏁 Script executed:
# Search for Процедура as a property being accessed on method objects
grep -r "\.Процедура\|Функция" "src/" --include="*.os" | grep -i "метод\|свойства"Repository: Stivo182/BenchmarkOneScript
Length of output: 53
🏁 Script executed:
# Look for any method type checking or filtering
grep -r "Если.*Процедура\|Если.*Функция" "src/" --include="*.os" | head -20Repository: Stivo182/BenchmarkOneScript
Length of output: 245
🏁 Script executed:
# Check if there's a test file that might show expected behavior
find "src/" -name "*тест*" -o -name "*Test*" -o -name "*test*" | head -20Repository: Stivo182/BenchmarkOneScript
Length of output: 53
🏁 Script executed:
# Search for documentation comments about method properties in reflector usage
grep -r "Процедура\|Функция" "src/" --include="*.os" -B 10 | grep -A 10 "ПолучитьТаблицуМетодов"Repository: Stivo182/BenchmarkOneScript
Length of output: 2093
🌐 Web query:
OneScript reflector ПолучитьТаблицуМетодов method table columns properties
💡 Result:
Для OneScript объекта Рефлектор метод «ПолучитьТаблицуМетодов» (GetMethodsTable) возвращает таблицу значений со следующими колонками: Имя, Количество, ЭтоФункция, Аннотации, Параметры, Экспорт. То есть, если ваша цель — узнать «колонки/свойства» table, которую возвращает Рефлектор.ПолучитьТаблицуМетодов(…): - Имя - Количество - ЭтоФункция - Аннотации - Параметры - Экспорт Если нужно, могу также подсказать, как аналогично устроена таблица свойств (ПолучитьТаблицуСвойств / GetPropertiesTable) в том же Рефлекторе.
Citations:
- 1: https://oscript.io/syntax/Reflector
- 2: https://github.com/EvilBeaver/OneScript.WebSite/blob/main/frontend/docs/syntax/Reflector.md
🏁 Script executed:
# Look for any README or documentation files that might explain the API
find "src/" -name "README*" -o -name "*.md" | head -20Repository: Stivo182/BenchmarkOneScript
Length of output: 53
🏁 Script executed:
# Check the root directory for any documentation
ls -la | head -30Repository: Stivo182/BenchmarkOneScript
Length of output: 1170
🏁 Script executed:
# Search for type checking patterns in the entire codebase
grep -r "ТипМетода\|Процедура\|метод.*Процедура" "src/" --include="*.os" -i | head -20Repository: Stivo182/BenchmarkOneScript
Length of output: 2222
Валидатор пропускает функции, хотя требует процедуры.
Ошибка указывает "процедурой", но проверка не различает функции и процедуры. Отсутствует проверка свойства ЭтоФункция из рефлектора — нужно добавить условие, гарантирующее, что метод является процедурой, а не функцией.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os`
around lines 324 - 334, В текущей проверке нужно гарантировать, что метод —
процедура, а не функция: дополнительно проверить свойство рефлектора
СвойстваМетода.ЭтоФункция и при его истинном значении возвращать Ложь; т.е. в
логике валидатора рядом с существующими проверками (СвойстваМетода.Экспорт и
проверкой ПараметрыМетода) добавить условие, которое отбрасывает методы, у
которых СвойстваМетода.ЭтоФункция = Истина, чтобы допускались только процедуры.
|
- проверять методы бенчмарков, поля параметров и источники параметров - учитывать источники параметров из внешних модулей - добавить интеграционные тесты валидации запуска
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/ТестыВалидацииЗапускаБенчмарков.os (2)
8-9: ⚡ Quick winДобавьте обязательную секцию
#Область ОбработчикиСобытий.В файле с тестами сейчас отсутствует одна из требуемых секций регионов.
♻️ Proposed fix
+ `#Область` ОбработчикиСобытий + + `#КонецОбласти` + `#Область` ТестыAs per coding guidelines:
tests/**/*.os: Use region structure in tests with sections:#Область ОбработчикиСобытий,#Область Тесты,#Область СлужебныеПроцедурыИФункции.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ТестыВалидацииЗапускаБенчмарков.os` around lines 8 - 9, Файл теста отсутствует обязательная секция регионов: добавьте заголовок региона "#Область ОбработчикиСобытий" (можно пустой блок если обработчиков нет) в структуру теста, сохранив порядок и наличие всех трех секций: "#Область ОбработчикиСобытий", "#Область Тесты" и "#Область СлужебныеПроцедурыИФункции" чтобы соответствовать требованиям для файлов tests/**/*.os; убедитесь, что названия секций точно совпадают с указанными символами.
10-96: ⚡ Quick winСведите однотипные тесты к параметризованному сценарию через
&Параметры(...).Здесь повторяется одна и та же структура шага с разными входами и ожидаемым текстом исключения; лучше объединить в один параметризованный тест.
As per coding guidelines:
tests/**/*.os: Prefer parameterized tests through&Параметры(...)when only input data and expectations change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ТестыВалидацииЗапускаБенчмарков.os` around lines 10 - 96, В этих тестах повторяется одна и та же проверка запуска с разными входными данными и ожидаемыми сообщениями; объедините процедуры ТестДолжен_ВыброситьИсключениеНаНепубличномМетодеБенчмарка, ТестДолжен_ВыброситьИсключениеНаНепубличномПолеПараметра, ТестДолжен_ВыброситьИсключениеНаНепубличномИсточникеОбщегоПараметра, ТестДолжен_ВыброситьИсключениеНаНепубличномИсточникеОбщегоПараметраИзМодуля и ТестДолжен_ВыброситьИсключениеНаНепубличномИсточникеПараметровБенчмарка в один параметризованный тест через &Параметры(...), перечислив сценарии с полями: дескрипторы (создавать через СоздатьКоллекциюСДескриптором), конфигурация (ТестированиеБенчмарков.КонфигурацияПоУмолчанию() плюс вызовы Конфигурация.ДобавитьПараметр или Конфигурация.ДобавитьИсточникПараметров или модификация дескриптора через ПолучитьПервый().ДобавитьИсточникПараметров) и ожидаемое сообщение; внутри параметризованного теста вызывать существующий помощник ПроверитьИсключениеЗапуска с параметрами из набора и удалить старые однотипные процедуры.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/ТестыВалидацииЗапускаБенчмарков.os`:
- Around line 8-9: Файл теста отсутствует обязательная секция регионов: добавьте
заголовок региона "#Область ОбработчикиСобытий" (можно пустой блок если
обработчиков нет) в структуру теста, сохранив порядок и наличие всех трех
секций: "#Область ОбработчикиСобытий", "#Область Тесты" и "#Область
СлужебныеПроцедурыИФункции" чтобы соответствовать требованиям для файлов
tests/**/*.os; убедитесь, что названия секций точно совпадают с указанными
символами.
- Around line 10-96: В этих тестах повторяется одна и та же проверка запуска с
разными входными данными и ожидаемыми сообщениями; объедините процедуры
ТестДолжен_ВыброситьИсключениеНаНепубличномМетодеБенчмарка,
ТестДолжен_ВыброситьИсключениеНаНепубличномПолеПараметра,
ТестДолжен_ВыброситьИсключениеНаНепубличномИсточникеОбщегоПараметра,
ТестДолжен_ВыброситьИсключениеНаНепубличномИсточникеОбщегоПараметраИзМодуля и
ТестДолжен_ВыброситьИсключениеНаНепубличномИсточникеПараметровБенчмарка в один
параметризованный тест через &Параметры(...), перечислив сценарии с полями:
дескрипторы (создавать через СоздатьКоллекциюСДескриптором), конфигурация
(ТестированиеБенчмарков.КонфигурацияПоУмолчанию() плюс вызовы
Конфигурация.ДобавитьПараметр или Конфигурация.ДобавитьИсточникПараметров или
модификация дескриптора через ПолучитьПервый().ДобавитьИсточникПараметров) и
ожидаемое сообщение; внутри параметризованного теста вызывать существующий
помощник ПроверитьИсключениеЗапуска с параметрами из набора и удалить старые
однотипные процедуры.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7879bd8-eb10-4186-a78a-2582d17a2ebe
📒 Files selected for processing (4)
src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.ostests/fixtures/benchmarks/Классы/БенчмаркСНепубличнымиЭлементами.ostests/fixtures/paramsSources/Модули/МодульСИсточникамиПараметров.ostests/ТестыВалидацииЗапускаБенчмарков.os
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/benchmarks/Классы/БенчмаркСНепубличнымиЭлементами.os
🚧 Files skipped from review as they are similar to previous changes (1)
- src/BenchmarkOneScript/core/running/Классы/ВалидаторЗапускаБенчмарков.os
- проверять имя модуля перед вызовом Вычислить - запретить небезопасные выражения в квалифицированном имени метода - покрыть разбор имени метода для объекта, модуля, типа и ошибок формата
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests