Skip to content

Conversation

@Y4nnikH
Copy link
Contributor

@Y4nnikH Y4nnikH commented Oct 20, 2025

See #3566

Also fix a number of type and other inconsistencies in the code.

@Y4nnikH Y4nnikH requested a review from tpurschke October 20, 2025 16:23
@Y4nnikH Y4nnikH self-assigned this Oct 20, 2025
Copy link
Contributor

@tpurschke tpurschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late reply - looking good for me. Is there a reason for this still being draft (apart from the sonar stuff)? What is the current status here?

@Y4nnikH Y4nnikH marked this pull request as ready for review October 25, 2025 15:57
Copilot AI review requested due to automatic review settings October 25, 2025 15:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enables fetching the latest configuration from management data stored in the database and addresses multiple type inconsistencies in the codebase. The changes standardize nullable field handling, rename properties for consistency (e.g., Rulesrules), and improve error handling throughout the import system.

Key Changes

  • Standardized nullable string fields across C# and Python models to properly handle optional data
  • Renamed Rules to rules for consistency with Python naming conventions
  • Enhanced hit information collection and processing with consolidated logic
  • Added validation for null values in critical paths and improved error messages

Reviewed Changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
roles/lib/files/FWO.Data/Rule.cs Changed InstallOn and Time from non-nullable with default empty strings to nullable strings
roles/lib/files/FWO.Data/NormalizedRulebaseLink.cs Added null validation for ToRulebase and LinkTypeObj before accessing properties
roles/lib/files/FWO.Data/NormalizedRulebase.cs Fixed JSON property name casing and removed null coalescing operators
roles/lib/files/FWO.Data/NormalizedRule.cs Changed multiple string fields to nullable and removed comment about null conversion
roles/lib/files/FWO.Data/NormalizedNetworkObject.cs Changed IP address fields to nullable strings
roles/lib/files/FWO.Data/NormalizedGateway.cs Updated multiple fields to nullable and assigned GlobalPolicyUid from device data
roles/lib/files/FWO.Data/LinkType.cs Changed Name from nullable to non-nullable with default value
roles/lib/files/FWO.Data/Device.cs Added GlobalRulebaseUid property
roles/importer/files/importer/test/tools/set_up_test.py Renamed Rules to rules throughout test utilities
roles/importer/files/importer/test/test_*.py Updated all test files to use lowercase rules property
roles/importer/files/importer/test/mocking/*.py Updated mock objects to use rules instead of Rules
roles/importer/files/importer/models/rulebase.py Changed Rules to rules and simplified RulebaseForImport
roles/importer/files/importer/models/rule.py Made multiple fields nullable and updated equality comparison to use model_dump
roles/importer/files/importer/models/gateway.py Changed gateway fields to nullable with appropriate defaults
roles/importer/files/importer/models/fwconfig_normalized.py Renamed and improved get_rulebase method to return Optional
roles/importer/files/importer/model_controllers/fwconfig_import_ruleorder.py Updated to use lowercase rules property
roles/importer/files/importer/model_controllers/fwconfig_import_rule.py Major refactoring: consolidated hit information collection, improved type hints, renamed methods to snake_case
roles/importer/files/importer/model_controllers/fwconfig_import.py Added null checks and improved error handling
roles/importer/files/importer/model_controllers/check_consistency.py Updated consistency checks to use lowercase rules
roles/importer/files/importer/fwo_api_call.py Removed deprecated update_hit_counter method
roles/importer/files/importer/fortiadom5ff/fmgr_rule.py Removed unused rule numbering logic and improved function signatures
roles/importer/files/importer/common.py Removed call to deleted update_hit_counter method
roles/importer/files/importer/checkpointR8x/cp_rule.py Added null checks for listOfGwUids and timeObjects
roles/database/files/upgrade/9.0.sql Added SQL to set deprecated rule_num field to 0
roles/common/files/fwo-api-calls/report/*.graphql Added GraphQL fields for fetching gateway configuration from database
roles/common/files/fwo-api-calls/import/rollbackImport.graphql Added rulebase_link deletion and restoration to rollback operation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Y4nnikH
Copy link
Contributor Author

Y4nnikH commented Oct 25, 2025

sorry for the late reply - looking good for me. Is there a reason for this still being draft (apart from the sonar stuff)? What is the current status here?

We should test this some more on large configs. And especially check with @alf-cactus that it works well for the production systems.

@tpurschke
Copy link
Contributor

@Y4nnikH do you think we could also test this PR with the ASA import on the customer machine? If so, could you please approve my latest PR, so that we can merge this and then test with the combined code from the two PRs?

Copy link
Contributor

@tpurschke tpurschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two points:

  1. might the rule_order issue found yesterday be related to #3838 (fixing the wrong bracket position in the screenshot might be a start)
    --> I tested with correct brackets but did not change anything
  2. see issue #3839

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 2, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
5 New issues

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants