Allow team names of length 36 to accommodate UUIDs #13097

@tobiasbp commented on GitHub (Jun 6, 2024):

Where in the code should the change be made?

@tobiasbp commented on GitHub (Jun 6, 2024): Where in the code should the change be made?

GiteaMirror commented

@delvh commented on GitHub (Jun 6, 2024):

modules/structs/org_team.go
27:     Name                    string `json:"name" binding:"Required;AlphaDashDot;MaxSize(30)"`
43:     Name                    string  `json:"name" binding:"AlphaDashDot;MaxSize(30)"`

I just had a look at the data model (/models/organization/team.go#79), and it looks like that already supports chars up to length 255.
So no need to change anything else

@delvh commented on GitHub (Jun 6, 2024): ```go modules/structs/org_team.go 27: Name string `json:"name" binding:"Required;AlphaDashDot;MaxSize(30)"` 43: Name string `json:"name" binding:"AlphaDashDot;MaxSize(30)"` ``` I just had a look at the data model (`/models/organization/team.go#79`), and it looks like that already supports chars up to length 255. So no need to change anything else

GiteaMirror commented

@silverwind commented on GitHub (Jun 6, 2024):

I assume xorm would auto-migrate the column to the new size, right? If yes, then it is just a change in those structs that is ideally accompanied by a small test that creates a team at the new maximum length.

@silverwind commented on GitHub (Jun 6, 2024): I assume xorm would auto-migrate the column to the new size, right? If yes, then it is just a change in those structs that is ideally accompanied by a small test that creates a team at the new maximum length.

GiteaMirror commented

@techknowlogick commented on GitHub (Jun 6, 2024):

@silverwind we require a migration for struct changes that would affect the DB, because even though automigrations will happen, we've run into cases in the past with too large of a version jump in the migration, then some auto-assumed migrations would be skipped (I haven't looked at this specific case though if it'd need the migration/struct change, but from above it seems it is just the validation that needs to be updated).

@techknowlogick commented on GitHub (Jun 6, 2024): @silverwind we require a migration for struct changes that would affect the DB, because even though automigrations will happen, we've run into cases in the past with too large of a version jump in the migration, then some auto-assumed migrations would be skipped (I haven't looked at this specific case though if it'd need the migration/struct change, but from above it seems it is just the validation that needs to be updated).

GiteaMirror commented

@delvh commented on GitHub (Jun 6, 2024):

I assume xorm would auto-migrate the column to the new size, right

As I said, not needed, it's already varchar 255

@delvh commented on GitHub (Jun 6, 2024): > I assume xorm would auto-migrate the column to the new size, right As I said, not needed, it's already `varchar 255`

GiteaMirror commented

@tobiasbp commented on GitHub (Jun 10, 2024):

So, is the only thing needed then to change these values to 255?

modules/structs/org_team.go
27:     Name                    string `json:"name" binding:"Required;AlphaDashDot;MaxSize(30)"`
43:     Name                    string  `json:"name" binding:"AlphaDashDot;MaxSize(30)"`

@tobiasbp commented on GitHub (Jun 10, 2024): So, is the only thing needed then to change these values to 255? ```` modules/structs/org_team.go 27: Name string `json:"name" binding:"Required;AlphaDashDot;MaxSize(30)"` 43: Name string `json:"name" binding:"AlphaDashDot;MaxSize(30)"` ````

GiteaMirror commented

@techknowlogick commented on GitHub (Jun 10, 2024):

@tobiasbp yes, although I'd suggest making the change locally, compiling, and testing to see if the names can then handle the additional length (in case we are missing another validation area).
I would recommend against bumping the length all the way up to 255 as some filesystems don't like really long paths.

@techknowlogick commented on GitHub (Jun 10, 2024): @tobiasbp yes, although I'd suggest making the change locally, compiling, and testing to see if the names can then handle the additional length (in case we are missing another validation area). I would recommend against bumping the length all the way up to 255 as some filesystems don't like really long paths.

GiteaMirror commented

@delvh commented on GitHub (Jun 10, 2024):

So, let's say 63?

@delvh commented on GitHub (Jun 10, 2024): So, let's say 63?

GiteaMirror commented

@silverwind commented on GitHub (Jun 11, 2024):

@tobiasbp yes, although I'd suggest making the change locally, compiling, and testing to see if the names can then handle the additional length (in case we are missing another validation area). I would recommend against bumping the length all the way up to 255 as some filesystems don't like really long paths.

255 is fine for all common file systems: https://github.com/sindresorhus/valid-filename/blob/main/index.js

@silverwind commented on GitHub (Jun 11, 2024): > @tobiasbp yes, although I'd suggest making the change locally, compiling, and testing to see if the names can then handle the additional length (in case we are missing another validation area). I would recommend against bumping the length all the way up to 255 as some filesystems don't like really long paths. 255 is fine for all common file systems: https://github.com/sindresorhus/valid-filename/blob/main/index.js

GiteaMirror commented

@techknowlogick commented on GitHub (Jun 11, 2024):

@silverwind the folder length is not enough to look at, some FS (in mobile so I can't link) look at the full path as well. I want to say it's exFAT, but it might be another (common one).
I ran into it on my synology, and iirc that's using some btrfs derivative (caveat, I was using an encrypted drive which added additional constraints)

@techknowlogick commented on GitHub (Jun 11, 2024): @silverwind the folder length is not enough to look at, some FS (in mobile so I can't link) look at the full path as well. I want to say it's exFAT, but it might be another (common one). I ran into it on my synology, and iirc that's using some btrfs derivative (caveat, I was using an encrypted drive which added additional constraints)

GiteaMirror commented

@silverwind commented on GitHub (Jun 11, 2024):

@tobiasbp yes, although I'd suggest making the change locally, compiling, and testing to see if the names can then handle the additional length (in case we are missing another validation area). I would recommend against bumping the length all the way up to 255 as some filesystems don't like really long paths.

255 is fine for all common file systems: https://github.com/sindresorhus/valid-filename/blob/main/index.js

Thought Windows does have a restriction called MAX_PATH that limits the full path length to 260 chars. I don't think team names can end up as filenames but for stuff that can (org name, repo name), I think 64 is a good limit.

@silverwind commented on GitHub (Jun 11, 2024): > > @tobiasbp yes, although I'd suggest making the change locally, compiling, and testing to see if the names can then handle the additional length (in case we are missing another validation area). I would recommend against bumping the length all the way up to 255 as some filesystems don't like really long paths. > > 255 is fine for all common file systems: https://github.com/sindresorhus/valid-filename/blob/main/index.js Thought Windows does have a [restriction called MAX_PATH](https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry) that limits the full path length to 260 chars. I don't think team names can end up as filenames but for stuff that can (org name, repo name), I think 64 is a good limit.

GiteaMirror commented