Refactor form property logic rule to load condition mapping from external JSON file #698
Conversation
WalkthroughThe pull request refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant F as FormPropertyLogicRule
participant FS as Filesystem (JSON)
F->>FS: Load data from resource_path("data/open_filters.json")
FS-->>F: Return JSON data
F->>F: Initialize $conditionMappingData with JSON data
F->>F: Use getConditionMapping() for condition type and comparator checks
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/app/Rules/FormPropertyLogicRule.php (2)
33-34: Consider adding a type-hint or docblock for$conditionMapping.Currently,
$conditionMappingis defined without indicating its type. Adding a typed property or a docblock can improve clarity and help static analysis tools.
97-97: Consider handling a missing'expected_type'.By defaulting to
nullwhen'expected_type'is absent, you rely on subsequent checks to catch this scenario. You might provide a clearer error message if'expected_type'is missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/app/Rules/FormPropertyLogicRule.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (3)
api/app/Rules/FormPropertyLogicRule.php (3)
83-83: Verify$this->conditionMappingis a valid array before referencing indices.If
json_decodefails or returnsnull, then$this->conditionMappingcould be non-array, and referencing$this->conditionMapping[$typeField]might cause a runtime warning.
90-90: Usage of$this->conditionMapping[$typeField]['comparators'][$operator]is well-guarded.This conditional check for the
'comparators'key is a good defensive approach. No further issues found.
119-120: Verify$this->field['type']exists before referencing$this->conditionMapping[$this->field['type']].If
$this->field['type']is not set or is unexpected, the array lookup can fail. You may want to ensure that$this->field['type']is always defined and valid before this line.
| public function __construct() | ||
| { | ||
| $this->conditionMapping = json_decode(file_get_contents(resource_path('data/open_filters.json')), true); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing or invalid JSON file.
If open_filters.json is missing or contains invalid JSON, $this->conditionMapping may remain empty or null. Consider throwing an exception or logging an error to avoid silent failures. For example:
public function __construct()
{
+ $jsonPath = resource_path('data/open_filters.json');
+ if (!file_exists($jsonPath)) {
+ throw new \RuntimeException("open_filters.json not found at: $jsonPath");
+ }
+ $content = file_get_contents($jsonPath);
+ $decoded = json_decode($content, true);
+ if (json_last_error() !== JSON_ERROR_NONE) {
+ throw new \RuntimeException("Failed to parse open_filters.json: " . json_last_error_msg());
+ }
+ $this->conditionMapping = $decoded;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct() | |
| { | |
| $this->conditionMapping = json_decode(file_get_contents(resource_path('data/open_filters.json')), true); | |
| } | |
| public function __construct() | |
| { | |
| $jsonPath = resource_path('data/open_filters.json'); | |
| if (!file_exists($jsonPath)) { | |
| throw new \RuntimeException("open_filters.json not found at: $jsonPath"); | |
| } | |
| $content = file_get_contents($jsonPath); | |
| $decoded = json_decode($content, true); | |
| if (json_last_error() !== JSON_ERROR_NONE) { | |
| throw new \RuntimeException("Failed to parse open_filters.json: " . json_last_error_msg()); | |
| } | |
| $this->conditionMapping = $decoded; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/app/Rules/FormPropertyLogicRule.php (1)
21-29:⚠️ Potential issueAdd error handling for JSON file operations.
The implementation needs robust error handling for file operations and JSON parsing.
Apply this diff to add error handling:
private static $conditionMappingData = null; public static function getConditionMapping() { if (self::$conditionMappingData === null) { + $jsonPath = resource_path('data/open_filters.json'); + if (!file_exists($jsonPath)) { + throw new \RuntimeException("open_filters.json not found at: $jsonPath"); + } + $content = file_get_contents($jsonPath); + if ($content === false) { + throw new \RuntimeException("Failed to read open_filters.json"); + } + $decoded = json_decode($content, true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \RuntimeException("Failed to parse open_filters.json: " . json_last_error_msg()); + } - self::$conditionMappingData = json_decode(file_get_contents(resource_path('data/open_filters.json')), true); + self::$conditionMappingData = $decoded; } return self::$conditionMappingData; }
🧹 Nitpick comments (2)
api/app/Rules/FormPropertyLogicRule.php (1)
121-122: Consider refactoring for better readability and error handling.The method could be improved with early returns and better error handling.
Apply this diff to improve the code:
if ($type === 'string' && isset(self::getConditionMapping()[$this->field['type']]['comparators'][$this->operator]['format'])) { - $format = self::getConditionMapping()[$this->field['type']]['comparators'][$this->operator]['format']; + $format = self::getConditionMapping()[$this->field['type']]['comparators'][$this->operator]['format'] ?? null; + if ($format === null) { + return false; + } if ($format['type'] === 'regex') { try { preg_match('/' . $value . '/', '');api/app/Rules/IntegrationLogicRule.php (1)
61-75: Consider caching the condition mapping.Multiple calls to
getConditionMapping()within the same method could impact performance.Apply this diff to improve performance:
+ $conditionMapping = FormPropertyLogicRule::getConditionMapping(); $typeField = $condition['value']['property_meta']['type']; $operator = $condition['value']['operator']; $value = $condition['value']['value']; - if (!isset(FormPropertyLogicRule::getConditionMapping()[$typeField])) { + if (!isset($conditionMapping[$typeField])) { $this->isConditionCorrect = false; $this->conditionErrors[] = 'configuration not found for condition type'; return; } - if (!isset(FormPropertyLogicRule::getConditionMapping()[$typeField]['comparators'][$operator])) { + if (!isset($conditionMapping[$typeField]['comparators'][$operator])) { $this->isConditionCorrect = false; $this->conditionErrors[] = 'configuration not found for condition operator'; return; } - $type = FormPropertyLogicRule::getConditionMapping()[$typeField]['comparators'][$operator]['expected_type']; + $type = $conditionMapping[$typeField]['comparators'][$operator]['expected_type'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/app/Rules/FormPropertyLogicRule.php(3 hunks)api/app/Rules/IntegrationLogicRule.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (1)
api/app/Rules/FormPropertyLogicRule.php (1)
85-99: LGTM! Good use of null coalescing operator.The changes correctly use the new
getConditionMapping()method, and the addition of the null coalescing operator forexpected_typeimproves robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/app/Rules/FormPropertyLogicRule.php (3)
21-29: Consider adding type hints and documentation.The new property and method lack type hints and documentation. Adding these would improve code maintainability and IDE support.
Apply this diff to add type hints and documentation:
- private static $conditionMappingData = null; + /** @var array<string,mixed>|null */ + private static $conditionMappingData = null; - public static function getConditionMapping() + /** + * Get the condition mapping from configuration. + * + * @return array<string,mixed> + * @throws \RuntimeException When condition mapping is not configured + */ + public static function getConditionMapping(): array { if (self::$conditionMappingData === null) { - self::$conditionMappingData = config('opnform.condition_mapping'); + $mapping = config('opnform.condition_mapping'); + if (!is_array($mapping)) { + throw new \RuntimeException('Condition mapping configuration is missing or invalid'); + } + self::$conditionMappingData = $mapping; } return self::$conditionMappingData; }
85-99: Consider extracting validation logic into separate methods.The condition validation logic in
checkBaseConditionis becoming complex. Consider extracting the validation into separate methods for better readability and maintainability.Apply this diff to extract validation logic:
+ /** + * Validate condition type field configuration. + * + * @param string $typeField + * @return bool + */ + private function validateConditionTypeField(string $typeField): bool + { + if (!isset(self::getConditionMapping()[$typeField])) { + $this->isConditionCorrect = false; + $this->conditionErrors[] = 'configuration not found for condition type'; + return false; + } + return true; + } + + /** + * Validate condition operator configuration. + * + * @param string $typeField + * @param string $operator + * @return bool + */ + private function validateConditionOperator(string $typeField, string $operator): bool + { + if (!isset(self::getConditionMapping()[$typeField]['comparators'][$operator])) { + $this->isConditionCorrect = false; + $this->conditionErrors[] = 'configuration not found for condition operator'; + return false; + } + return true; + } private function checkBaseCondition($condition) { // ... existing validation code ... $typeField = $condition['value']['property_meta']['type']; $operator = $condition['value']['operator']; $this->operator = $operator; $value = $condition['value']['value']; - if (!isset(self::getConditionMapping()[$typeField])) { - $this->isConditionCorrect = false; - $this->conditionErrors[] = 'configuration not found for condition type'; - - return; - } - - if (!isset(self::getConditionMapping()[$typeField]['comparators'][$operator])) { - $this->isConditionCorrect = false; - $this->conditionErrors[] = 'configuration not found for condition operator'; - - return; - } + if (!$this->validateConditionTypeField($typeField)) { + return; + } + + if (!$this->validateConditionOperator($typeField, $operator)) { + return; + } $type = self::getConditionMapping()[$typeField]['comparators'][$operator]['expected_type'] ?? null; // ... rest of the method }
121-122: Consider caching the mapping lookup results.The code repeatedly accesses the condition mapping configuration. Consider caching the lookup results to improve performance.
Apply this diff to cache the mapping lookup:
private function valueHasCorrectType($type, $value) { - if ($type === 'string' && isset(self::getConditionMapping()[$this->field['type']]['comparators'][$this->operator]['format'])) { - $format = self::getConditionMapping()[$this->field['type']]['comparators'][$this->operator]['format']; + $mapping = self::getConditionMapping(); + $fieldType = $this->field['type']; + if ($type === 'string' && isset($mapping[$fieldType]['comparators'][$this->operator]['format'])) { + $format = $mapping[$fieldType]['comparators'][$this->operator]['format'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/app/Rules/FormPropertyLogicRule.php(3 hunks)api/config/opnform.php(1 hunks)api/vapor.yml(2 hunks)docker/php-fpm-entrypoint(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (5)
docker/php-fpm-entrypoint (2)
61-64: LGTM! Well-structured optimization function.The new
optimize_application()function follows the established patterns with proper logging and clear purpose.
13-14: LGTM! Logical placement of optimization step.The optimization step is correctly placed after project initialization but before server startup, maintaining a proper initialization sequence.
api/vapor.yml (3)
27-28: LGTM! Production deployment optimization properly configured.The optimization step is correctly placed after migrations in the production environment deployment sequence.
45-46: LGTM! Staging deployment optimization properly configured.The optimization step is correctly placed after migrations in the staging environment deployment sequence.
13-14: Verify memory settings for optimization step.The optimization process may require additional memory during deployment. While the current memory settings (2048MB for production, 1024MB for staging) should be sufficient, monitor the deployment logs to ensure there are no memory-related issues during the optimization step.
Also applies to: 33-34
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check deployment logs for memory-related issues during optimization # Search for memory-related errors or warnings in recent deployments gh run list --workflow=deploy.yml --limit 5 --json conclusion,url | \ jq -r '.[] | select(.conclusion != "success") | .url' | \ while read -r url; do gh run view "$url" --log | grep -i "memory\|out of memory\|killed" doneLength of output: 233
Action: Verify the workflow identifier used for memory log retrieval
The script referenced a workflow file nameddeploy.yml, which resulted in an HTTP 404 error. This indicates that either the workflow file has a different name or isn’t available, meaning the memory log check couldn’t be performed automatically. Please manually verify that the correct workflow is being targeted for the optimization step and confirm that the deployment logs are effectively capturing any memory-related issues.
| @@ -6,4 +6,5 @@ | |||
| 'template_editor_emails' => explode(',', env('TEMPLATE_EDITOR_EMAILS') ?? ''), | |||
| 'extra_pro_users_emails' => explode(',', env('EXTRA_PRO_USERS_EMAILS') ?? ''), | |||
| 'show_official_templates' => env('SHOW_OFFICIAL_TEMPLATES', true), | |||
| 'condition_mapping' => json_decode(file_get_contents(resource_path('data/open_filters.json')), true), | |||
There was a problem hiding this comment.
Add error handling for JSON file loading.
The configuration entry directly loads and decodes the JSON file without any error handling. If the file is missing or contains invalid JSON, it could cause the application to fail during startup.
Apply this diff to add error handling:
- 'condition_mapping' => json_decode(file_get_contents(resource_path('data/open_filters.json')), true),
+ 'condition_mapping' => function() {
+ $jsonPath = resource_path('data/open_filters.json');
+ if (!file_exists($jsonPath)) {
+ throw new \RuntimeException("open_filters.json not found at: $jsonPath");
+ }
+ $content = file_get_contents($jsonPath);
+ if ($content === false) {
+ throw new \RuntimeException("Failed to read open_filters.json");
+ }
+ $decoded = json_decode($content, true);
+ if (json_last_error() !== JSON_ERROR_NONE) {
+ throw new \RuntimeException("Failed to parse open_filters.json: " . json_last_error_msg());
+ }
+ return $decoded;
+ }(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'condition_mapping' => json_decode(file_get_contents(resource_path('data/open_filters.json')), true), | |
| 'condition_mapping' => function() { | |
| $jsonPath = resource_path('data/open_filters.json'); | |
| if (!file_exists($jsonPath)) { | |
| throw new \RuntimeException("open_filters.json not found at: $jsonPath"); | |
| } | |
| $content = file_get_contents($jsonPath); | |
| if ($content === false) { | |
| throw new \RuntimeException("Failed to read open_filters.json"); | |
| } | |
| $decoded = json_decode($content, true); | |
| if (json_last_error() !== JSON_ERROR_NONE) { | |
| throw new \RuntimeException("Failed to parse open_filters.json: " . json_last_error_msg()); | |
| } | |
| return $decoded; | |
| }(), |
…rnal JSON file
Summary by CodeRabbit