fix: enforce valid sketch folder name on `Save as` #1821

kittaakos · 2023-01-17T16:02:59Z

Motivation

IDE2 must disallow the creation of sketches with invalid folder names: https://arduino.github.io/arduino-cli/dev/sketch-specification/#sketch-root-folder

Change description

IDE2 prompts every invalid sketch folder name inside the sketchbook on startup. IDE2 raises the dialog only in the first window after the start.
No sketches with invalid folder names are visible under File > Sketchbook.
When the user changes the sketchbook location from the preferences, and the new sketchbook location contains invalid sketch folders, IDE2 raises the warning dialog for every invalid sketch, if any. IDE2 warns the user in the window where the sketchbook path has changed. The other windows do not warn the user if multiple windows are opened.
When the user saves a sketch (Save As...), IDE2 does not allow the save if the sketch folder name is invalid. IDE2 opens a dialog and warns the user. The user can decide whether to edit the new sketch folder name or cancel the operation.
All newly introduced dialog titles and messages are the same as in IDE 1.x.
The remote sketch creation uses the new validation rules and logic, but no behavioral changes are expected.

I had a hard time recording screencasts that fit the max upload size, so no teasers this time.

Other information

Closes #1599

Reviewer checklist

PR addresses a single concern.
The PR has no duplicates (please search among the Pull Requests before creating one)
PR title and description are properly filled.
Docs have been added / updated (for bug fixes / features)

per1234

UPDATE: resolved

It is fine if you want to consider it outside the stated scope of this PR, which is about the sketch folder name enforcement for "Save As..." operations, but in order to fully resolve https://github.com//issues/1599, the IDE must also enforce valid names when adding or renaming additional sketch code files via "**New Tab**" and "**Rename**" of the editor toolbar context menu.

Select File > New Sketch from the Arduino IDE menus.

Click the ●●● icon on the right side of the editor toolbar.

Select "New Tab" from the menu.

Type ``~!@#$%^&()=+[{]};',.cpp` in the "Name for new file" dialog field.

Click the "OK" button.
🐛 The IDE allowed the addition of a sketch code file with non-compliant name.

Select Tools > Board > Arduino AVR Boards > Arduino Uno from the Arduino IDE menus.

Select Sketch > Verify/Compile from the Arduino IDE menus.
🐛 Compilation fails with a cryptic error:
avr-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-58EC7305E05EBCCAD711E866CB3F08F6\sketch\`~!@#$%^&()=+[;',.cpp: No such file or directory avr-g++: warning: '-x c++' after last input file has no effect avr-g++: fatal error: no input files compilation terminated. exit status 1 Compilation error: exit status 1

kittaakos · 2023-01-23T09:28:26Z

No problem, I can add the validation when creating a new file. #1599 does not mention it. Where can I find the validation constraints for the other files? Besides the additional code files and the allowed extensions, IDE2 needs to know how to deal with non-code files, such as .adoc, .json, etc. and what are the permitted filenames.

I have noticed that the spec, IDE2 code, and the CLI are not in sync regarding the allowed code file extensions:

Spec: https://arduino.github.io/arduino-cli/dev/sketch-specification/#additional-code-files

IDE2:

arduino-ide/arduino-ide-extension/src/common/protocol/sketches-service.ts

Lines 175 to 179 in 6140ae5

    
           export const MAIN = ['.ino', '.pde']; 
        
           export const SOURCE = ['.c', '.cpp', '.S']; 
        
           export const CODE_FILES = [...MAIN, ...SOURCE, '.h', '.hh', '.hpp']; 
        
           export const ADDITIONAL = [...CODE_FILES, '.json', '.md', '.adoc']; 
        
           export const ALL = Array.from(new Set([...MAIN, ...SOURCE, ...ADDITIONAL]));

CLI: https://github.com/arduino/arduino-cli/blob/a58d5adbe269ecc55778c007dc8ac39ffd42ce85/arduino/globals/globals.go#L31-L58

Would you like for me to adjust them?

arduino-ide-extension/src/browser/contributions/sketchbook.ts

per1234 · 2023-01-23T10:07:56Z

Where can I find the validation constraints for the other files? Besides the additional code files and the allowed extensions, IDE2 needs to know how to deal with non-code files, such as .adoc, .json, etc. and what are the permitted filenames.

I don't think any validation is needed for these files because they don't have any technical usage (with the exception of the special JSON files with fixed filenames, for which name validation is not applicable).

Would you like for me to adjust them?

That would be wonderful.

kittaakos · 2023-01-23T10:13:51Z

I don't think any validation is needed for these files because they don't have any technical usage

Thank you! What do you think about the code files? Should they follow the sketch folder name validation constraints?

That would be wonderful.

Sure.

kittaakos · 2023-01-23T11:27:01Z

Per, when this PR gets merged, IDE2 will filter all sketches in invalid sketch folders (File > Sketchbook). What should IDE2 do when there is a code file with an invalid name in the sketch? Thank you!

per1234 · 2023-01-23T12:37:19Z

What do you think about the code files? Should they follow the sketch folder name validation constraints?

Yes. This is already required by the specification:

https://arduino.github.io/arduino-cli/dev/sketch-specification/#sketch-folders-and-files

The sketch root folder name and code file names must ...

What should IDE2 do when there is a code file with an invalid name in the sketch?

Arduino IDE 1.x shows a notification only when the sketch is opened:

File name foo bar.cpp is invalid: ignored

However, that is a lie. The file is not ignored as promised.

I think the best approach is to check for and treat invalid sketch code filenames the same as you are already doing for invalid sketch folder names. Either way it is an invalid sketch so I don't think there is a benefit from handling the two in different ways from a UX standpoint.

kittaakos · 2023-01-23T12:46:35Z

don't think there is a benefit from handling the two in different ways from a UX standpoint.

Thanks. So IDE2 should filter them.

I think the best approach is to check for and treat invalid sketch code filenames the same as you are already doing for invalid sketch folder names.

IDE2 does not use the CLI to discover the sketch folders in the sketchbook. IDE2 uses a glob pattern. When IDE2 detects an invalid sketch folder, it warns the users and filters the sketch from File > Sketchbook when the window loads.

IDE2 does not know which files are associated with a sketch. The CLI provides this info via the LoadSketchRequest. If IDE2 wants to filter all sketches which in invalid folder name or contains invalid code files, IDE2 must load all sketches before app startup. IDE2 can do it, but it will be super slow.

per1234 · 2023-01-23T13:00:27Z

OK, then I guess the only option is to validate the sketch files when the sketch is loaded and show the invalid sketch dialog then.

Thinking of it more, this approach actually makes the most sense because the user may open sketches that are outside the sketchbook.

Maybe it would be best to also postpone the folder name validation until the sketch is opened?

kittaakos · 2023-01-23T13:29:40Z

OK, then I guess the only option is to validate the sketch files when the sketch is loaded and show the invalid sketch dialog then.

This approach is possible. IDE2 raises a warning dialog regarding the invalid sketch code files when the sketch is opened. IDE2 won't be able to filter those sketches from File > Sketchbook, which contains invalid code files.

Thinking of it more, this approach actually makes the most sense because the user may open sketches that are outside the sketchbook.

It is unrelated to whether the sketch is in the sketchbook or not. Users can open any sketches, any file that ends .ino or a folder that contains at least one .ino file will open, but IDE2 will prompt for a move to fix the sketch folder name when the browser window opens.

Maybe it would be best to also postpone the folder name validation until the sketch is opened?

This is what IDE2 will do if this PR gets merged. IDE2 opens a sketch and warns about all invalid sketch folder names inside the sketchbook. This happens only when the sketch is opened in IDE2. What IDE2 cannot do easily is warn about all sketches which contain invalid code files. The latter would require loading all sketches when any sketch opens.

IDE2 cannot do easily

Actually, IDE2 can do it quickly, but it would be bad for the startup performance, which is not the greatest anyway.

per1234 · 2023-01-23T14:06:00Z

This is what IDE2 will do if this PR gets merged. IDE2 opens a sketch and warns about all invalid sketch folder names inside the sketchbook.

I meant that it would only validate the folder name of the specific sketch that was opened, just as it will only validate filenames in the specific sketch that was opened.

per1234 · 2023-01-25T02:08:39Z

arduino-ide-extension/src/common/protocol/sketches-service.ts

+  export function validateCloudSketchFolderName(
+    candidate: string
+  ): string | undefined {
+    return /^[0-9a-zA-Z_]{1,36}$/.test(candidate)


We need to get a confirmation that it really is necessary to disallow the - and . characters in Cloud sketches. That seems a completely unnecessary departure from the Arduino Sketch Specification to me.

I started an internal dialog.

per1234 · 2023-01-25T02:12:37Z

arduino-ide-extension/src/common/protocol/sketches-service.ts

+  export function validateCloudSketchFolderName(
+    candidate: string
+  ): string | undefined {
+    return /^[0-9a-zA-Z_]{1,36}$/.test(candidate)


Suggested change

return /^[0-9a-zA-Z_]{1,36}$/.test(candidate)

return /^[0-9a-zA-Z]{1}[0-9a-zA-Z_]{0,35}$/.test(candidate)

I'm ready to accept that there is some hypothetical reason why the maximum length must be more restrictive for Cloud sketch. At least in this case a valid cloud sketch name length is still in compliance with the specification, even though the converse is not true.

But it is unacceptable to allow Cloud sketch names that are in violation of the Arduino Sketch Specification.

But it is unacceptable to allow Cloud sketch names that are in violation of the Arduino Sketch Specification.

It was requested in one of the design files that IDE2 must rename sketches when cloning from and pushing to the cloud. We need to add tests to verify IDE2 can do the name transformation.

Users can still create and pull a remote sketch, so relaxing the regex here might not help. What do you think?

Users can still create and pull a remote sketch, so relaxing the regex here might not help. What do you think?

I don't think it is a problem because the new approach is to validate names when the sketch is opened. So even if an invalid sketch is pulled, it will still be at the time the user opens the pulled sketch.

I'm referring to this feature:

#1821 (comment)

When you open an invalid sketch, IDE2 will prompt for user actions to fix the issues.

First, IDE2 prompts for the sketch folder name fix as a Save as... operation. IDE2 prompt until the user specifies a valid folder name or aborts the operation.

If the sketch folder name has been corrected, IDE2 reloads and IDE2 validates the code files

If there are any invalid code files, the user has to fix them as a Rename operation.

When all problems have been fixed, IDE2 is ready for use. If no, IDE2 prompts users whether they want to interrupt the operation and navigate to a new temp sketch.

I don't think it is a problem because the new approach is to validate names when the sketch is opened. So even if an invalid sketch is pulled, it will still be at the time the user opens the pulled sketch.

You're correct, but it only covers some user cases. After IDE2 pulls the cloud sketch and fixes any possible sketch folder name and code file name errors, the user can create a file remotely and pull it again. It will be spec incompatible if the name of the code file starts with an underscore. Please correct me if I am overlooking something. Thanks!

IDE2 may run a validation after pulling from the remote.

the user can create a file remotely and pull it again. It will be spec incompatible if the name of the code file starts with an underscore. Please correct me if I am overlooking something.

You are right that this would provide a path to having a non-compliant sketch open in Arduino IDE. However, that will only be possible during that session. The next time the user tries to open the sketch, the added file will be validated.

So I wouldn't consider this scenario to completely invalidate the proposed approach of only doing the filename validation on an open operation.

Ideally validation would also be done when a file is added or renamed through an "external" process such as a pull. There is an equivalent scenario for local sketches in that the user may add or rename files of the open sketch externally. But I don't consider such validation to be absolutely essential. The most important reason for validating against the specification is to ensure shared sketches are compatible with any official Arduino development tool as well as 3rd party tools from conscientious developers. I think it would be rare that a developer would not end up later opening the type of sketch that is shared (as opposed to a quick throwaway test sketch we might write and use during a single IDE session and never look at again).

So if it is relatively easy to add a validation triggered by external addition or name change of sketch file, then great. But if this is something of significant difficulty to implement then I think the "on open" approach is sufficient.

Closes #1599 Co-authored-by: Akos Kitta <[email protected]> Co-authored-by: per1234 <[email protected]> Signed-off-by: Akos Kitta <[email protected]>

kittaakos · 2023-01-25T10:46:30Z

As discussed, I completely rewrote the implementation.

Expectations:

File > Sketchbook shows all invalid sketches with an invalid folder name or any invalid code files. /path/to/sketch/Sketch.ino is not visible.
Nothing happens when you open a valid sketch and the sketchbook contains invalid sketches.
When you open an invalid sketch, IDE2 will prompt for user actions to fix the issues.
- First, IDE2 prompts for the sketch folder name fix as a Save as... operation. IDE2 prompt until the user specifies a valid folder name or aborts the operation.
- If the sketch folder name has been corrected, IDE2 reloads and IDE2 validates the code files
- If there are any invalid code files, the user has to fix them as a Rename operation.
- When all problems have been fixed, IDE2 is ready for use. If no, IDE2 prompts users whether they want to interrupt the operation and navigate to a new temp sketch.
From now on, IDE2 allows saving new sketches with a valid folder name.
From now on, IDE2 allows creating/renaming code files that comply with the spec.
This PR does not change anything regarding the cloud sketch name validations. It's still an ongoing discussion.

Please let me know if you have better wording for the dialog titles and messages.
I am open to changing the order of the user actions: fix files first, then the folder, but the windows reload is inevitable.

Please review. Thanks!

per1234

I'm experiencing unexpected results when opening Cloud sketches with invalid name:

Download the sketch ZIP file from this link:
Invalid(Name).zip
Use Arduino Web Editor to import it into your Cloud sketchbook:
https://create.arduino.cc/editor
Sync Cloud sketchbook in Arduino IDE.
Pull the "Invalid(Name)" sketch.
Open the "Invalid(Name)" sketch.
🙂 I get a dialog informing me the sketch has an invalid name.
😕 But the dialog tells me the local sketch name restrictions (i.e., max length 63) instead of the Cloud sketch restrictions (i.e., max length 36):
Click the "OK" button.
🙂 I get a "Save sketch folder as..." dialog
😕 But the dialog is for saving it as a local sketch.
Download the sketch ZIP file from this link:
InvalidAdditionalFileName.zip
Use Arduino Web Editor to import it into your Cloud sketchbook:
https://create.arduino.cc/editor
Sync Cloud sketchbook in Arduino IDE.
Pull the "InvalidAdditionalFileName" sketch.
Open the "InvalidAdditionalFileName" sketch.
🙂 I get a dialog informing me the sketch has a file with invalid name.
😕 But the dialog tells me the local sketch name restrictions (i.e., max length 63) instead of the Cloud sketch restrictions (i.e., max length 36):
Click the "OK" button.
🙂 I get a rename file dialog
😕 But my impression is that renaming Cloud sketch files is not currently supported: Enable rename and delete options for Cloud Sketch and/or any of its files #1825
Type InvalidAdditionalFileName12345678901234567890123456789012345678890.h in the sketch filename field.
😕 The error message tells me the local sketch name restrictions (i.e., max length 63) instead of the Cloud sketch restrictions (i.e., max length 36):
Change the name to InvalidAdditionalFileName12345678901234567890123456789012345678
😕 The dialog considers it a valid name, even though it is longer than the 36 character Cloud sketch maximum.
Click the "OK" button.
🙂 The sketch is still in my Cloud sketchbook.
Push the sketch.
🙂 The push was successful.

kittaakos · 2023-01-27T08:45:40Z

Thank you for the review. This PR depends on #1833. I will not update this PR, but I won't close it either until the other one is ready for review.

kittaakos · 2023-02-02T16:15:38Z

I'm experiencing unexpected results when opening Cloud sketches with invalid name:

#1833 should handle these.

kittaakos · 2023-02-06T12:56:12Z

Closing as duplicate. Please review #1833 instead. The other PR must comply to all the requirements this PR meant to cover. Thank you!

per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jan 18, 2023

kittaakos force-pushed the #1599 branch from 3248c3d to 2cb9d64 Compare January 19, 2023 09:36

kittaakos requested review from per1234 and AlbyIanna January 19, 2023 09:46

kittaakos marked this pull request as ready for review January 19, 2023 10:02

per1234 reviewed Jan 23, 2023

View reviewed changes

per1234 requested changes Jan 23, 2023

View reviewed changes

arduino-ide-extension/src/browser/contributions/sketchbook.ts Outdated Show resolved Hide resolved

kittaakos marked this pull request as draft January 23, 2023 10:14

This was referenced Jan 24, 2023

Creation of sketches with non-compliant names is permitted #1599

Closed

The supported file extensions of additional code files are not in sync with the spec arduino/arduino-cli#2050

Closed

kittaakos force-pushed the #1599 branch from 20c5de4 to 60a64b1 Compare January 24, 2023 17:14

per1234 requested changes Jan 25, 2023

View reviewed changes

fix: enforce valid sketch folder name on Save as

2aea3df

Closes #1599 Co-authored-by: Akos Kitta <[email protected]> Co-authored-by: per1234 <[email protected]> Signed-off-by: Akos Kitta <[email protected]>

kittaakos force-pushed the #1599 branch from 60a64b1 to 2aea3df Compare January 25, 2023 10:21

kittaakos marked this pull request as ready for review January 25, 2023 12:21

per1234 requested changes Jan 27, 2023

View reviewed changes

kittaakos added the status: on hold Do not proceed at this time label Jan 27, 2023

kittaakos marked this pull request as draft February 3, 2023 15:57

kittaakos closed this Feb 6, 2023

kittaakos added conclusion: duplicate Has already been submitted and removed status: on hold Do not proceed at this time labels Feb 6, 2023

kittaakos deleted the #1599 branch February 6, 2023 12:59

per1234 mentioned this pull request Feb 8, 2023

feat: rename, deletion, and validation support #1833

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enforce valid sketch folder name on `Save as` #1821

fix: enforce valid sketch folder name on `Save as` #1821

kittaakos commented Jan 17, 2023 •

edited

Loading

per1234 left a comment •

edited

Loading

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

kittaakos commented Jan 23, 2023

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

per1234 Jan 25, 2023 •

edited

Loading

kittaakos Jan 25, 2023

per1234 Jan 25, 2023

kittaakos Jan 25, 2023

per1234 Jan 27, 2023

kittaakos Feb 2, 2023

per1234 Feb 3, 2023

kittaakos commented Jan 25, 2023

per1234 left a comment •

edited

Loading

kittaakos commented Jan 27, 2023

kittaakos commented Feb 2, 2023

kittaakos commented Feb 6, 2023

	return /^[0-9a-zA-Z_]{1,36}$/.test(candidate)
	return /^[0-9a-zA-Z]{1}[0-9a-zA-Z_]{0,35}$/.test(candidate)

fix: enforce valid sketch folder name on Save as #1821

fix: enforce valid sketch folder name on Save as #1821

Conversation

kittaakos commented Jan 17, 2023 • edited Loading

Motivation

Change description

Other information

Reviewer checklist

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

kittaakos commented Jan 23, 2023

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

kittaakos commented Jan 23, 2023

per1234 commented Jan 23, 2023

per1234 Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

kittaakos Jan 25, 2023

Choose a reason for hiding this comment

per1234 Jan 25, 2023

Choose a reason for hiding this comment

kittaakos Jan 25, 2023

Choose a reason for hiding this comment

per1234 Jan 27, 2023

Choose a reason for hiding this comment

kittaakos Feb 2, 2023

Choose a reason for hiding this comment

per1234 Feb 3, 2023

Choose a reason for hiding this comment

kittaakos commented Jan 25, 2023

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

kittaakos commented Jan 27, 2023

kittaakos commented Feb 2, 2023

kittaakos commented Feb 6, 2023

fix: enforce valid sketch folder name on `Save as` #1821

fix: enforce valid sketch folder name on `Save as` #1821

kittaakos commented Jan 17, 2023 •

edited

Loading

per1234 left a comment •

edited

Loading

per1234 Jan 25, 2023 •

edited

Loading

per1234 left a comment •

edited

Loading