Skip to content

Fix csv imports' carelessness #9933

Closed
sopex wants to merge 4 commits intoopnsense:masterfrom
sopex:importt
Closed

Fix csv imports' carelessness #9933
sopex wants to merge 4 commits intoopnsense:masterfrom
sopex:importt

Conversation

@sopex
Copy link
Copy Markdown
Member

@sopex sopex commented Mar 12, 2026

Fixes: #9861
And many more CSV imports.

In my testing, it works well with all the CSV imports. It might be a bit simplistic, but it is great for blocking careless mistakes.

@AdSchellevis
Copy link
Copy Markdown
Member

let's not cut off partial imports, it's perfectly fine to only offer required values as long as it passes validations.

@sopex
Copy link
Copy Markdown
Member Author

sopex commented Mar 12, 2026

@AdSchellevis

IMO, validations should not allow the config backup to be imported as dhcp entries or firewall rules. And firewall rules should not be allowed to be imported as dhcp entries :)

Thus, a more comprehensive validation could be created per instance to catch more things but this PR prevents the more crude errors without over-validating everything, in a sense, promoting what you suggest.

Also, the error message tells you what you are missing. If you know what you are doing you just add them manually.

@AdSchellevis
Copy link
Copy Markdown
Member

If the model supports empty records to be added, it's easy to render new (empty) records indeed, which might be less practical, but also a bit part of the deal I'm afraid. I don't mind looking at options to at least require something being inserted, the question just is what that should be. requiring keys doesn't really make it less fragile as in time a lot of these things have similar keys like a uuid.

@sopex
Copy link
Copy Markdown
Member Author

sopex commented Mar 12, 2026

If you believe that more than one CSV uses exactly the same keys then this doesn't make much sense. In my experience, they don't, but haven't really looked it up.

Requiring something to be inserted is not a bad idea, but it will break many already saved files :(

@sopex sopex closed this Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Firewall: Import/Export add header check to .csv

2 participants