Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate all headers/fields when bulk import objects #11617

Closed
julianstolp opened this issue Jan 30, 2023 · 5 comments · Fixed by #13826
Closed

Validate all headers/fields when bulk import objects #11617

julianstolp opened this issue Jan 30, 2023 · 5 comments · Fixed by #13826
Assignees
Labels
severity: low Does not significantly disrupt application functionality, or a workaround is available status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@julianstolp
Copy link

NetBox version

v3.4.3

Python version

3.8

Steps to Reproduce

  1. Open Prefix Bulk Import
  2. Specify all required field options and add some dummy data
    prefix,status,testa,testb
    10.10.10.0/24,active,a,b
  3. Press Import
  4. Import is done without any complains

Expected Behavior

Fields which are not stated in Field Options should throw an error, or a warning that some data was ignored.

Observed Behavior

Import is done without any complains, Looks like all data was imported correct, although some fields were ignored

@julianstolp julianstolp added the type: bug A confirmed report of unexpected behavior in the application label Jan 30, 2023
@jeremystretch jeremystretch added the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label Jan 30, 2023
@kkthxbye-code
Copy link
Contributor

It this a regression? Did it used to throw an error? Are we sure this is actually wanted behavior before I look into this?

@julianstolp
Copy link
Author

I just tried it in v3.09. It raises a Unexpected column header "testa" found. error.

@julianstolp
Copy link
Author

Also checked versions 3.2.0, 3.3.0 and 3.4.0. The import works with version 3.4.0, before raises an Unexpected column header "testa" found. error.

@kkthxbye-code kkthxbye-code self-assigned this Feb 8, 2023
@kkthxbye-code kkthxbye-code added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation labels Feb 8, 2023
@kkthxbye-code kkthxbye-code removed their assignment Feb 8, 2023
@kkthxbye-code kkthxbye-code added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation and removed status: accepted This issue has been accepted for implementation labels Feb 8, 2023
@kkthxbye-code
Copy link
Contributor

@arthanson - Can you take a look at this as it's part of the bulk import stuff you changed.

I tried just adding the following validation on L147 here:

class CSVModelForm(forms.ModelForm):
"""
ModelForm used for the import of objects in CSV format.
"""
def __init__(self, *args, headers=None, fields=None, **kwargs):
headers = headers or {}
fields = fields or []
super().__init__(*args, **kwargs)
# Modify the model form to accommodate any customized to_field_name properties
for field, to_field in headers.items():
if to_field is not None:
self.fields[field].to_field_name = to_field
# Omit any fields not specified (e.g. because the form is being used to
# updated rather than create objects)
if fields:
for field in list(self.fields.keys()):
if field not in fields:
del self.fields[field]

for header in headers.keys():
    if header not in self.fields:
        raise forms.ValidationError(f'Unexpected column header "{header}" found.')

But the ValidationError is caught here and discarded:

except (AbortTransaction, ValidationError):
clear_webhooks.send(sender=self)

@arthanson arthanson self-assigned this Mar 20, 2023
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation labels Apr 10, 2023
@jeremystretch
Copy link
Member

We should consider JSON/YAML imports as well as CSV. If we're concerned about checking for extraneous CSV headers (and IMO we should be), then this functionality should exist for JSON- and YAML-formatted data as well. Unlike CSV, which specifies a single row of headers, the fields for each object expressed in JSON or YAML can vary with each object, so each object will need to be validated individually. I think it's reasonable to replicate this pattern for CSV data, even if it's not technically necessary.

@jeremystretch jeremystretch added the severity: low Does not significantly disrupt application functionality, or a workaround is available label Jun 23, 2023
@jeremystretch jeremystretch changed the title Prefix Bulk Import Validate all headers/fields when bulk import objects Sep 15, 2023
jeremystretch added a commit that referenced this issue Sep 20, 2023
* Fixes #11617: Check for invalid CSV headers during bulk import

* Add test for CSV import header validation
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
severity: low Does not significantly disrupt application functionality, or a workaround is available status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
4 participants