Skip to content
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

relax restriction on handle length in PDS #2392

Closed
wants to merge 2 commits into from

Conversation

itaru2622
Copy link
Contributor

fix #2391

@itaru2622
Copy link
Contributor Author

the code followed the discussion in the issue #2391.

@bnewbold bnewbold requested a review from dholms April 11, 2024 16:54
@bnewbold
Copy link
Collaborator

@itaru2622 this is simple and resolves a long-standing issue for self-hosted PDS operators. thanks!

could you add a couple additional tests to handle-validation.test.ts, for this new logic?

for example, a longer base domain in the domains list, and an example of a less-than-18-char handle working with that (even if longer than 30 total chars), and a ~20 char handle on the .test domain (which would be less than 30 chars total, but still invalid). you'd have to add a helper for "expectNotThrow" or similar I think.

if you have trouble adding these LMK and I an take a pass at it.

@itaru2622
Copy link
Contributor Author

@bnewbold test case added in handle-validation.test.ts

I hope below tests meet your request. please refer the code for detail.

    expectThrow('usernamepartover18c.test',    'Handle too long')                                                                                                        
    expectNotThrow('u23456789012345678.test',  'safe up to 18 chars in first segment of the handle')                                                                     
    expectThrow('usernamepartover18c.loooooooooooooooooong-pds-over18chars.mybsky.mydomain.com',   'Handle too long')                                                    
    expectNotThrow('u23456789012345678.loooooooooooooooooong-pds-over18chars.mybsky.mydomain.com', 'safe long domain in the handle')

Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet this looks good 👍

Thanks for the tests as well

@bnewbold
Copy link
Collaborator

Two CI test issues:

  • lint issues with the new test code
  • many of the example handles in pds/tests/seeds/users-bulk.ts have become invalid and need to be updated; this impacts tests in other packages that use those accounts

I can try to resolve both of these from a new branch, and will update this PR when I get around to that.

@bnewbold
Copy link
Collaborator

I can't update this PR (the branch is in a different repo), but I created this new PR on top of it, which resolves the build issues: #2392

@bnewbold
Copy link
Collaborator

@itaru2622 I merged the above-linked PR, which included your commits. I'm going to close this PR now.

It may take us a while to update the PDS "distribution".

@itaru2622
Copy link
Contributor Author

@bnewbold thank you for your work on this PR and fixing.

I confirmed my codes were merged into latest main branch.
so, I will close this PR.

@itaru2622
Copy link
Contributor Author

This PR has been taken over by #2410 and has been merged into main branch, so this PR is closed.

@itaru2622 itaru2622 closed this Apr 16, 2024
@itaru2622 itaru2622 deleted the fix_issue2391 branch April 29, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handle length restriction is too short.
3 participants