efcore icon indicating copy to clipboard operation
efcore copied to clipboard

feat: normalize migrations file

Open BirajMainali opened this issue 2 years ago • 6 comments

  • [x] I've read the guidelines for contributing and seen the walkthrough

If we use the reserve key character while adding a new migration it doesn't work as aspected for eg dotnet ef migrations add "add: user", this tries to create a migration file same as provided name, but if the migrations name includes a reserve key characters for the file name, I don't know how but it's an unknown migrations file.

Steps to reproduce

  • Adding initial migration including symbol : which is not allowed to include on the file name. image
  • It creates an unknown file, some times it will be hard to remove from the machine. image

So, I've tried to fix this issue, I apologize if this brings the breaking changes.

  • [ ] Tests for the changes have been added (for bug fixes/features)
  • [x] Code follows the same patterns and style as existing code in this repo

BirajMainali avatar Jul 05 '22 11:07 BirajMainali

@BirajMainali Thanks for your interest in this issue. Rather than generating an error, the invalid input should be sanitized so that it can still be used. Also, remember that we can't accept a PR without appropriate test coverage.

ajcvickers avatar Jul 09 '22 18:07 ajcvickers

@bricelam, can you please suggest to me, where should i include the sensitization test. based on my last commit.

BirajMainali avatar Jul 13 '22 14:07 BirajMainali

CLA assistant check
All CLA requirements met.

dnfadmin avatar Jul 14 '22 02:07 dnfadmin

For tests, you can add them to test/EFCore.Design.Tests/Migrations/Design/MigrationScaffolderTest.cs.

The TempDirectory test helper will probably come in handy for these.

// Creates a temporary directory that gets deleted at the end of the test
using var projectDir = new TempDirectory();
scaffolder.Save(projectDir, migration, outputDir: null);

bricelam avatar Jul 14 '22 21:07 bricelam

@bricelam, I've changed the methodology to identify invalid file name characters. At previous changes I've used Path.GetInvalidFileNameChars() method to get invalid chars, but in .net 6 this told that only / is the invalid character for a file which is not enough. I've visited issue regarding provider method named Path.GetInvalidFileNameChars() on https://github.com/dotnet/runtime/issues/63383

I found some of the common and used invalid file name characters to sensitize the migrations file name. And it just works as expected. In case of other characters are found we can simply include them on the array.

BirajMainali avatar Jul 15 '22 16:07 BirajMainali

Any reason this isn't getting merged? @bricelam

BirajMainali avatar Aug 17 '22 08:08 BirajMainali

@bricelam, @ajcvickers

BirajMainali avatar Nov 09 '22 06:11 BirajMainali

We discussed this again and we're concerned about platform differences making this unreliable and/or overly aggressive, especially since migrations may be used on a different platform than the one on which they were generated. The value here seems minimal, so instead of doing this people should create migration names that avoid using special characters, unless they are confident that these characters will work for their target platform(s).

ajcvickers avatar Mar 30 '23 12:03 ajcvickers

We discussed this again and we're concerned about platform differences making this unreliable and/or overly aggressive, especially since migrations may be used on a different platform than the one on which they were generated. The value here seems minimal, so instead of doing this people should create migration names that avoid using special characters unless they are confident that these characters will work for their target platform(s).

@ajcvickers, @roji I see this issue from a different view, In our office workspace we use a Windows machine for developing work and I and other team members use Linux at home, so in this case we are facing this issue. we cannot directly pull and start working due to this file system issue. Linux users can name the migrations like feat: add product rate in this case windows users cannot work with that migration file. Also, we are not normalizing every normal migration file only in case of special cases we are normalizing special characters by _ so this will not bring any issues.

BirajMainali avatar Jan 20 '24 14:01 BirajMainali