Multiple improvements and fixing most open issues #14

SasLuca · 2023-12-30T13:24:59Z

Fixed most open issues and did some nice improvements. Given the popularity of this library and as a fellow user of it and nanoid contributor (I wrote zig-nanoid) I think it would be cool to reach 0 open issues and improve the library like this.

Below I detail everything I did.

Summary

Added useful alphabets from nanoid-dictionary so users don't have to maintain their own in most cases.
Made the default id size a public constant so people can easily reference it, this is constant across all nanoid implementations.
Improved or added missing docs to all public members.
Fixed various typos and exception instantiations.
Fixed issue Method Clz32 can be simplified on .NET 7 or greater #12 by using Int32.LeadingZeroCount and adding .net7 as a target for unit tests.
Fixed issue Each new instance of CryptoRandom replaces the static generator. #9 by making CryptoRandom._r a non-static field.
Fixed issue Add table giving a quick reference of collision probabilities? #5 by adding more information in readme.md
Minor changes to formatting.

Remarks

If this is merged, I also recommend fixing issue License URL Missing from NuGet Package #13. It seems like while the license file is specified in the csproj, nuget doesn't just pick it up from there. The MS docs from what I've seen suggest using a .nuspec file but I never published on nuget before so I am not sure how that works.
I also tried looking into issue Nanoid.dll from net45 targeting NuGet is flagged as malicious file by Cynet #7 but it is not clear to me on that website what the problem with this library is. Only one vendor seems to tag the library as malicious but I can't see any rationale for it. I suggest either asking for more info from the person who opened the issue (assuming they have more knowledge on the topic and care about it) and see if there is a fix. If not, probably fine to just close.
If any of the formatting or name changes are undesirable I can also change those, I tried not to change too much in that regard though, so I think it should be ok.

Breaking changes

The way in which CryptoRandom works is technically a breaking change, as _r is no longer static, but I would argue that no one should have been abusing that implementation detail anyways, as #9 mentions, so in practice this shouldn't break anyone's code.

Testing

I compiled and ran the tests using .net8 and they all passed.

- Added useful alphabets from nanoid-dictionary. - Made the default id size a public constant so people can easily reference it. - Fixed various typos and improved summaries and exception instantiations. - Fixed issue codeyu#12 by using Int32.LeadingZeroCount. - Fixed issue codeyu#9 by making CryptoRandom._r a non-static field. - Fixed issue codeyu#5 by adding more information in readme.md - Minor changes to formatting.

…into improvements # Conflicts: # src/Nanoid/Nanoid.cs

SasLuca · 2023-12-30T13:25:38Z

README.md

@@ -64,6 +64,10 @@ var random = Random(10);
 var id = Nanoid.Generate(random, "abcdef", 10) //=> "fbaefaadeb"
 ```

+### Collision calculator


This added documentation fixes #5

SasLuca · 2023-12-30T13:26:38Z

nanoid-net.sln.DotSettings

Added some common words that were viewed as typos to the shared dictionary of the project.

SasLuca · 2023-12-30T13:27:01Z

src/Nanoid/CryptoRandom.cs

    /// </summary>
    public class CryptoRandom : Random
    {
-        private static RandomNumberGenerator _r;
-#if !NETSTANDARD2_1
+        private readonly RandomNumberGenerator _r;


SasLuca · 2023-12-30T13:28:07Z

src/Nanoid/Nanoid.cs

-        /// <param name="size"></param>
-        /// <returns></returns>
-        public static async Task<string> GenerateAsync(string alphabet = DefaultAlphabet, int size = 21)
+        public static class Alphabets


A collection of useful nanoid alphabets, mostly taken from nanoid-dictionary. I don't think making a separate package for these would make sense. At best we could extra them into another file but I think they are fine sitting here.

SasLuca · 2023-12-30T13:29:09Z

src/Nanoid/Nanoid.cs

+        /// <summary>
+        /// The default size of a Nanoid.
+        /// </summary>
+        public const int DefaultIdSize = 21;


DefaultIdSize is now public for ease of reference. For example I needed this constant when using the MaxLength attribute on a nanoid.

SasLuca · 2023-12-30T13:31:12Z

src/Nanoid/Nanoid.cs

            if (alphabet == null)
            {
-                throw new ArgumentNullException("alphabet cannot be null.");
+                throw new ArgumentNullException(nameof(alphabet), "alphabet cannot be null.");


I got warnings that the recommended way to instantiate exceptions like this one is to pass nameof(argument) as the first parameter so did so here.

Not sure if the message "alphabet cannot be null" is needed anymore tho.

SasLuca · 2023-12-30T13:31:28Z

src/Nanoid/Nanoid.cs

+
+            // Use `Int32.LeadingZeroCount` on .net7 and above
+            #if NET7_0
+            var mask = (2 << 31 - Int32.LeadingZeroCount(alphabet.Length - 1 | 1)) - 1;


SasLuca · 2023-12-30T13:32:06Z

test/Nanoid.Test/Nanoid.Test.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFrameworks>netcoreapp2.0;netcoreapp3.0</TargetFrameworks>
+    <TargetFrameworks>netcoreapp2.0;netcoreapp3.0;net7</TargetFrameworks>


Added in order to run the unit tests and make sure they still pass when using Int32.LeadingZeroCount in .net7 and above.

SasLuca · 2023-12-30T13:40:12Z

Not sure why the CI fails, reading the logs it seems to be because it can't find an appropriate .net version or something? Haven't used appveyor for this before so not sure, maybe a contributor that worked with this can look into it.

irby · 2023-12-30T21:59:25Z

I'm picking up Nanoid for a project I'm building and a lot of these improvements are exactly what I'm looking for (in particular, an accessible alphabet).

Thank you for you work on this!

- Resolved thread safety issues - Improved README.md

SasLuca · 2024-01-02T12:17:28Z

I identified an additional problem and documented it here: #15

Also implemented a fix and added it as part of this pull request.

If there is some detail that I missed which makes this issue irrelevant let me know but it seems like a real problem from everything I read and my solution I believe fixes it.

SasLuca · 2024-01-02T12:20:35Z

src/Nanoid/Nanoid.cs

+        /// <remarks>
+        /// Lazily initialized in order to account for the ThreadStatic attribute (learn more here: https://stackoverflow.com/a/18086509)
+        /// </remarks>
+        public static CryptoRandom GlobalRandom => _globalRandom ?? (_globalRandom = new CryptoRandom());


Initializing _globalRandom directly in my understanding would not behave as expected when using [ThreadStatic] as it uses the static constructor of the class which is only called once in the lifetime of the entire program, whereas what we want here is to initialize the variable per thread when it is used. Thus the recommended solution based on my reading is to have a getter such as this one that lazily initializes it upon usage.

I also made it public in case there are people who wish to use this but need to preload it. I haven't personally measured the initialization of CryptoRandom but I think this enables people who think it might be an issue to deal with it without requiring more changes to the library.

codeyu · 2024-02-24T15:11:17Z

@SasLuca Thank your PR, I'll merge this PR first, releasing the new version may take a few more days. Thanks you, @SasLuca.

MichaelHochriegl · 2024-05-12T17:59:36Z

@SasLuca Thank your PR, I'll merge this PR first, releasing the new version may take a few more days. Thanks you, @SasLuca.

Hey @codeyu

is a new release scheduled any time soon? I'm currently trying out NanoIds and could really use these changes in your awesome lib.

SasLuca added 3 commits December 30, 2023 00:56

Merge branch 'improvements' of https://github.com/SasLuca/nanoid-net …

272257c

…into improvements # Conflicts: # src/Nanoid/Nanoid.cs

SasLuca commented Dec 30, 2023

View reviewed changes

SasLuca changed the title ~~Multiple improvements and fixing most open issue~~ Multiple improvements and fixing most open issues Dec 30, 2023

Fixed codeyu#15

cadf2e0

- Resolved thread safety issues - Improved README.md

SasLuca commented Jan 2, 2024

View reviewed changes

Added example usages of Nanoid.Alphabets to README.md

deaeef4

codeyu merged commit 208fcc7 into codeyu:master Feb 24, 2024
1 check failed

This was referenced Feb 27, 2024

Method Clz32 can be simplified on .NET 7 or greater #12

Closed

Each new instance of CryptoRandom replaces the static generator. #9

Closed

Add table giving a quick reference of collision probabilities? #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple improvements and fixing most open issues #14

Multiple improvements and fixing most open issues #14

SasLuca commented Dec 30, 2023 •

edited

Loading

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca Dec 30, 2023

SasLuca commented Dec 30, 2023

irby commented Dec 30, 2023

SasLuca commented Jan 2, 2024

SasLuca Jan 2, 2024

codeyu commented Feb 24, 2024

MichaelHochriegl commented May 12, 2024

Multiple improvements and fixing most open issues #14

Multiple improvements and fixing most open issues #14

Conversation

SasLuca commented Dec 30, 2023 • edited Loading

Summary

Remarks

Breaking changes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SasLuca commented Dec 30, 2023

irby commented Dec 30, 2023

SasLuca commented Jan 2, 2024

Choose a reason for hiding this comment

codeyu commented Feb 24, 2024

MichaelHochriegl commented May 12, 2024

SasLuca commented Dec 30, 2023 •

edited

Loading