GUNDI-4878: Field mapping creation script v1.0 #397
Conversation
vgarcia13
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a Django management command to automate the creation and updating of field mapping configurations between source and destination integrations in Gundi v2.
Key Changes:
- Added
field_mappings_creation_script.pymanagement command with CLI arguments for connection ID, destination ID, and provider key - Implements RouteConfiguration creation/update logic with field mappings
- Includes transaction handling and error reporting for integration lookups
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Outdated
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Outdated
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Outdated
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Outdated
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Outdated
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Show resolved
Hide resolved
cdip_admin/integrations/management/commands/field_mappings_creation_script.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with transaction.atomic(): | ||
| try: | ||
| source_integration = Integration.objects.get( | ||
| id=connection_id, | ||
| ) | ||
| except Integration.DoesNotExist: | ||
| self.stdout.write(f" -- ERROR: Connection ID {connection_id} does not exist in Gundi v2 -- ") | ||
| return | ||
|
|
||
| try: | ||
| destination_integration = Integration.objects.get( | ||
| id=destination_id, | ||
| ) | ||
| except Integration.DoesNotExist: | ||
| self.stdout.write(f" -- ERROR: Destination ID {destination_id} does not exist in Gundi v2 -- ") | ||
| return |
There was a problem hiding this comment.
The transaction will be rolled back on error (due to the transaction.atomic() context), but the script returns early without rolling back when integrations are not found (lines 54-55, 62-63). This creates inconsistent behavior where some errors cause rollback and others don't. Additionally, returning inside a transaction block that will be exited normally means the transaction commits successfully even though an error was reported. Consider raising an exception or using a different error handling strategy to ensure consistent transaction handling.
| self.stdout.write(f" -- ERROR: Connection ID {connection_id} does not exist in Gundi v2 -- ") | ||
| return | ||
|
|
||
| try: | ||
| destination_integration = Integration.objects.get( | ||
| id=destination_id, | ||
| ) | ||
| except Integration.DoesNotExist: | ||
| self.stdout.write(f" -- ERROR: Destination ID {destination_id} does not exist in Gundi v2 -- ") |
There was a problem hiding this comment.
Error messages on lines 54 and 62 are written to self.stdout instead of self.stderr. Error messages should be written to stderr for proper error handling and to allow users to separate normal output from error output. Change these to self.stderr.write().
|
|
||
| class Command(BaseCommand): | ||
|
|
||
| help = "Gundi v2 field mapping creation script" |
There was a problem hiding this comment.
[nitpick] The help text for this command is generic and doesn't clearly explain what the script does or when it should be used. Consider adding a more descriptive help text that explains the purpose of creating field mappings between integrations and what the expected outcome is. For example: "Creates or updates field mapping configuration between a source integration and destination integration using a provider key."
| help = "Gundi v2 field mapping creation script" | |
| help = ( | |
| "Creates or updates field mapping configuration between a source integration and a " | |
| "destination integration using a provider key. " | |
| "This command should be used to establish or modify how fields are mapped from the source " | |
| "integration to the destination integration in Gundi v2. " | |
| "Expected outcome: the specified field mapping is created or updated for the given integrations." | |
| ) |
| field_mappings = { | ||
| str(source_integration.id): { | ||
| "obv": {} |
There was a problem hiding this comment.
[nitpick] The nested structure uses a hardcoded key "obv" without explanation of what it represents or why this specific key is used. Add a comment explaining the meaning and purpose of this key in the field mapping structure, or consider using a named constant to make the code more self-documenting.
| route_config, created = RouteConfiguration.objects.get_or_create( | ||
| name=f"{source_integration.default_route.name} (Integration ID: {source_integration.id}) - Default Config", | ||
| defaults={ | ||
| "data": field_mappings_result | ||
| } | ||
| ) |
There was a problem hiding this comment.
Using get_or_create with only the name field as the unique identifier can be problematic if multiple integrations could have the same route name pattern. This could lead to unintended sharing of RouteConfiguration objects between different integrations. Consider adding a unique constraint or using the integration ID in a more unique way (e.g., as part of a unique_together constraint with name, or using update_or_create with a more specific lookup).
| '--provider-key', | ||
| type=str, | ||
| required=True, | ||
| help='Provider key to be used in field mapping creation [REQUIRED]' |
There was a problem hiding this comment.
[nitpick] Inconsistent quote style: The first two arguments use double quotes for the argument name (lines 18, 24), but the third argument uses single quotes (line 30). For consistency, use double quotes for all argument names, or follow the project's style guide if it specifies a preference.
| '--provider-key', | |
| type=str, | |
| required=True, | |
| help='Provider key to be used in field mapping creation [REQUIRED]' | |
| "--provider-key", | |
| type=str, | |
| required=True, | |
| help="Provider key to be used in field mapping creation [REQUIRED]" |
Related Link:
https://allenai.atlassian.net/browse/GUNDI-4878
This pull request introduces a new Django management command script to automate the creation and updating of field mappings between integrations in Gundi v2.
The script ensures that the correct field mapping configuration is set up between a source and destination integration using a provided provider key.
Key additions and features:
New Management Command for Field Mappings:
field_mappings_creation_script.py) that creates or updates field mapping configurations between integrations, using command-line arguments for source connection ID, destination ID, and provider key.RouteConfigurationobject with the correct field mappings and associates it with the source integration's default route.