UrlEncodingParser doesn't allow null values

[DaveSweeton]

We're using UrlEncodingParser as a replacement for HttpUtility.ParseQueryString (thank you!). One difference we've noticed is that when calling ToString on the returned NameValueCollection is that it throws exceptions when a value is null, unlike the System.Web implementation.

System.ArgumentNullException : Value cannot be null.
Parameter name: stringToEscape
   at System.Uri.EscapeDataString(String stringToEscape)
   at Westwind.Utilities.UrlEncodingParser.ToString() in D:\projects\Westwind.Utilities\Westwind.Utilities\SupportClasses\UrlEncodingParser.cs:line 132

[DaveSweeton]

It's an easy fix, I'm happy to submit a PR!

[RickStrahl]

Thanks - it's easy enough to fix, no need for a PR.

Fixed and now this works:

        [TestMethod]
        public void QueryStringNullTest()
        {
            var query = new UrlEncodingParser(null);                        
            Assert.IsTrue(query.Count == 0);

            query.Set("id", "3123");
            Assert.IsTrue(query.Count == 1);

            Assert.IsTrue(query["id"] == "3123");

            query = new UrlEncodingParser(null);
            var nvCol = query.Parse(null);

            Assert.IsTrue(nvCol != null);
            Assert.IsTrue(nvCol.Count == 0);
        }

You weren't specific but I assume it was passing null into the ctor or Parse() was the concern?

[DaveSweeton]

Thanks! No, it was adding a null value after parse: query["name"] = null, then using query.ToString().

-Dave

On Tue, Jun 11, 2024, 8:06 PM Rick Strahl @.***> wrote:

Thanks - it's easy enough to fix, no need for a PR.

Fixed and now this works:

    [TestMethod]
    public void QueryStringNullTest()
    {
        var query = new UrlEncodingParser(null);
        Assert.IsTrue(query.Count == 0);

        query.Set("id", "3123");
        Assert.IsTrue(query.Count == 1);

        Assert.IsTrue(query["id"] == "3123");

        query = new UrlEncodingParser(null);
        var nvCol = query.Parse(null);

        Assert.IsTrue(nvCol != null);
        Assert.IsTrue(nvCol.Count == 0);
    }

You weren't specific but I assume it was passing null into the ctor or Parse() was the concern?

— Reply to this email directly, view it on GitHub https://github.com/RickStrahl/Westwind.Utilities/issues/24#issuecomment-2161821880, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4XET7Z7JWYPMDWX7AURVLZG6GGXAVCNFSM6AAAAABJEJSKAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRHAZDCOBYGA . You are receiving this because you authored the thread.Message ID: @.***>

[RickStrahl]

Ah Ok. Ok that's fixed too 😀

        [TestMethod]
        public void QueryValuesNullTest()
        {
            var query = new UrlEncodingParser(null);
            query["test"] = "1234";
            query["id"] = null;

            var qs= query.ToString();

            // test=1234&id=
            Assert.IsTrue(qs.Contains("id="));
        }

[RickStrahl]

Actually I wonder if the value is null the key probably shouldn't be there at all. Empty too? Maybe not there with null, but there but empty with string.empty.

[DaveSweeton]

I think with the system.web version the key is present and the value is empty: ?name=&id=26

-Dave

On Tue, Jun 11, 2024, 8:19 PM Rick Strahl @.***> wrote:

Actually I wonder if the value is null the key probably shouldn't be there at all. Empty too? Maybe not there with null, but there but empty with string.empty.

— Reply to this email directly, view it on GitHub https://github.com/RickStrahl/Westwind.Utilities/issues/24#issuecomment-2161833573, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4XET5QIFV5R47P6ZAXOJ3ZG6HZDAVCNFSM6AAAAABJEJSKAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRHAZTGNJXGM . You are receiving this because you authored the thread.Message ID: @.***>

[RickStrahl]

Yeah I can handle that in parsing, but if you put an explicit null into the an indexer you're going to get it back.

But that actually does answer my question. I can handle the parse but I have to know that the value exists in order to return it in the collection. So it has to be there - otherwise the indexer will fail and I can't control that.

I wrote this so long ago, and it was definitely not the greatest idea to subclass NVC :smile:

[RickStrahl]

Thinking this over the only fix that's really needed is this:

        /// <summary>
        /// override indexer to ensure we always non-null value
        /// </summary>
        public string this[string key]
        {
            get
            {
                return base[key] ?? string.Empty;                
            }
            set
            {
                base[key] = value ?? string.Empty;
            }
        }

This ensures that:

• Null values are always fixed up on the way in and out
• Missing values also return string.empty

This changes behavior, but it's consistent and in line what's expected from the System.Web components so I'm OK with it.

Unlikely that this would be a problem for many - if you use this thing 99% of the time you'd be passing in a full query string value to parse out the values and any parsed value will always have a string value. The only time null is/was an issue is when explicitly assigned via the indexer.

[DaveSweeton]

That probably works for my situation, but I wonder if it would be better to check for null before calling System.Uri.EscapeDataString? That way you get back what you put in via the indexer, but ToString returns the expected result.

On Tue, Jun 11, 2024, 8:06 PM Rick Strahl @.***> wrote:

Thanks - it's easy enough to fix, no need for a PR.

Fixed and now this works:

    [TestMethod]
    public void QueryStringNullTest()
    {
        var query = new UrlEncodingParser(null);
        Assert.IsTrue(query.Count == 0);

        query.Set("id", "3123");
        Assert.IsTrue(query.Count == 1);

        Assert.IsTrue(query["id"] == "3123");

        query = new UrlEncodingParser(null);
        var nvCol = query.Parse(null);

        Assert.IsTrue(nvCol != null);
        Assert.IsTrue(nvCol.Count == 0);
    }

You weren't specific but I assume it was passing null into the ctor or Parse() was the concern?

— Reply to this email directly, view it on GitHub https://github.com/RickStrahl/Westwind.Utilities/issues/24#issuecomment-2161821880, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4XET7Z7JWYPMDWX7AURVLZG6GGXAVCNFSM6AAAAABJEJSKAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRHAZDCOBYGA . You are receiving this because you authored the thread.Message ID: @.***>

[RickStrahl]

The value can never be null now because the assignment now ensures that it won't be and the parser always sets a non-null value anyway.

I had that code initially but it didn't address the null value when:

• returning a null value explicitly set into the collection (ie. not parsed)
• returning a null value when returning a bogus key

The indexer addresses all those scenarios.

[DaveSweeton]

Sounds good! My main scenario was to build a string value, so my code didn't read from the indexer.

On Tue, Jun 11, 2024, 9:06 PM Rick Strahl @.***> wrote:

The value can never be null now because the assignment now ensures that it won't be and the parser always sets a non-null value anyway.

I had that code initially but it didn't address the null value when:

• returning a null value explicitly set into the collection (ie. not parsed)
• returning a null value when returning a bogus key

The indexer addresses all those scenarios.

— Reply to this email directly, view it on GitHub https://github.com/RickStrahl/Westwind.Utilities/issues/24#issuecomment-2161870973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4XETZNELJK3XREHKAY2OTZG6NJJAVCNFSM6AAAAABJEJSKAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRHA3TAOJXGM . You are receiving this because you authored the thread.Message ID: @.***>