-
Notifications
You must be signed in to change notification settings - Fork 812
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
Table name not protected against spaces #1951
Comments
I haven't contributed to ClosedXML in a while, trying to get back into it, would be willing to work on this. Would the desired behavior here be to throw an exception? |
Alright, I looked into this, obviously it's fairly easy to add a check, but I don't want to without sign off from one of the main contributors. The reason being is, adding a check for a space, would actually result in a change in the existing behavior that might cause a backwards incompatibility for users. Currently during workbook save when serializing the TablePartDefinitionXml there is a call to GetTableName which has a call to RemoveSpecialCharacters which is normalizing the table name by removing any spaces. This means the saved file is not corrupt. For if example if I set the table name to "Table With Spaces" it will be saved as "TableWithSpaces" Fixing this would mean we break existing users in the wild if they were relying on this behavior Not fixing it means you have hidden side effects if you do a round trip. Meaning if you open the saved file you generated with ClosedXml you can't access the table by the name "Table With Spaces" you would have to know the internals stripped away spaces and would have to access it by the normalized named "TableWithSpaces" @jahav what behavior would you prefer? |
Make the get table by name functionality use the same functionality so that when table name with spaces or other weirdness is sent in, it fixes the key for the developer. |
In the scenario that I was using was important that the name is correct. I think the throwing error is the most correct. Is important the code has a coherent answer to dev errors. But this is my opinion. |
I think it would be better to reject invalid name with an exception. Name of a table is not just an identifier, but it can also be used formulas structured references (e.g. I am in favour of simple, understandable interfaces and contracts and principle of least surprise. Having a name sanitizer inside is not in line with that. I would therefore refuse any invalid table names. We already throw an exception Please add a note/mitigation about change to the https://github.com/ClosedXML/ClosedXML/blob/develop/docs/migrations/migrate-to-0.101.rst + info in XML doc. Spec says
|
Read and complete the full issue template
Do not randomly delete sections. They are here for a reason.
Do you want to request a feature or report a bug?
Did you test against the latest CI build?
If you answered
No
, please test with the latest development build first.Version of ClosedXML
0.97.0
What is the current behavior?
Allows empty space in table names
What is the expected behavior or new feature?
When you give a name to a table the code protect against starting with number or repeated names, but don't protected against space
The expected behavior is to give an error
Is this a regression from the previous version?
I don't know is my first attempt to use the library
The text was updated successfully, but these errors were encountered: