Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Adding HTTP method constants #712

Merged
merged 11 commits into from
Sep 27, 2016
Merged

Adding HTTP method constants #712

merged 11 commits into from
Sep 27, 2016

Conversation

mikaelm12
Copy link
Contributor

For issue #693
@Tratcher @muratg

public const string Put = "PUT";
public const string Trace = "TRACE";

public static bool Equals(string firstMethod, string secondMethod)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? Where are we going to use it?

@davidfowl
Copy link
Member

a more optimized approach would be to have methods for each of the verbs that does a reference equality before doing the case insensitive matching.

Since we're declaring static strings we may as well use them....

@davidfowl
Copy link
Member

Should these be consts or static readonly?

@muratg muratg added this to the Backlog milestone Sep 23, 2016
@muratg
Copy link

muratg commented Sep 23, 2016

Let's not do this in 1.1. We'll consider this later.


public static bool IsConnectMethod(string inputMethod)
{
return object.ReferenceEquals(Connect ,inputMethod) | StringComparer.OrdinalIgnoreCase.Equals(Connect, inputMethod);
Copy link
Member

@davidfowl davidfowl Sep 23, 2016

Choose a reason for hiding this comment

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

Wrong or, we don't want bitwise or here. You need ||.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Connect , -> Connect,

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidfowl I think you mean ||.

Copy link
Member

Choose a reason for hiding this comment

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

I edited my comment 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you said "you need | " I figured you meant an extra one

Copy link
Member

Choose a reason for hiding this comment

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

Yasss

@davidfowl
Copy link
Member

@muratg why?

@muratg
Copy link

muratg commented Sep 23, 2016

@davidfowl unnecessary churn, no?

@davidfowl
Copy link
Member

Seems minimal and we have similar things like header names. It would be even better if have Kestrel use these:

/cc @halter73 @CesarBS

return object.ReferenceEquals(Connect ,inputMethod) | StringComparer.OrdinalIgnoreCase.Equals(Connect, inputMethod);
}

public static bool IsDeleteMethod(string inputMethod)
Copy link
Member

Choose a reason for hiding this comment

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

Change inputMethod to method

@cesarblum
Copy link
Contributor

Why do case-insensitive comparison when the RFC says method names are case-sensitive? https://tools.ietf.org/html/rfc7230#section-3.1.1

@Tratcher
Copy link
Member

@CesarBS because in practice they are case-insensitive.

@@ -4,6 +4,7 @@
using System.Collections.Generic;
using Microsoft.AspNetCore.Http.Features;
using Xunit;
using Microsoft.AspNetCore.Http;
Copy link
Member

Choose a reason for hiding this comment

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

Sort usings

{
return object.ReferenceEquals(Trace, method) || StringComparer.OrdinalIgnoreCase.Equals(Trace, method);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

we should still have the Equals(string, string) method

@@ -55,5 +55,10 @@ public static bool IsTraceMethod(string method)
{
return object.ReferenceEquals(Trace, method) || StringComparer.OrdinalIgnoreCase.Equals(Trace, method);
}

public static bool Equals(string left, string right)
Copy link
Member

Choose a reason for hiding this comment

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

This method is pointless... We hard code a few methods there's no point having a case insensitive overload here. It adds nothing..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna let you and @Tratcher settle this...

Copy link
Member

Choose a reason for hiding this comment

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

@Tratcher lets decide on this offline.

@khellang
Copy link
Contributor

Where's HEAD?

@mikaelm12
Copy link
Contributor Author

Its there now 😄. Good catch

return object.ReferenceEquals(Get, method) || StringComparer.OrdinalIgnoreCase.Equals(Get, method);
}

public static bool IsHeadMethod(string method){
Copy link
Member

Choose a reason for hiding this comment

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

formatting?

return object.ReferenceEquals(Delete, method) || StringComparer.OrdinalIgnoreCase.Equals(Delete, method);
}

public static bool IsGetMethod(string method)
Copy link
Member

Choose a reason for hiding this comment

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

HttpMethod.IsGetMethod(string) seems redundant. What about just HttpMethod.IsGet(string)?


namespace Microsoft.AspNetCore.Http
{
public static class HttpMethods
Copy link
Member

Choose a reason for hiding this comment

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

Should this be singular?
HttpMethod.Get
HttpMethod.IsGet(string)
HttpMethod.Equals(string, string)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. This is similar to a non-flag enum.

return object.ReferenceEquals(Trace, method) || StringComparer.OrdinalIgnoreCase.Equals(Trace, method);
}

public static bool Equals(string left, string right)
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? If anything this could be private and called by the Is* methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a big discussion on this. We're taking it out

Copy link
Member

Choose a reason for hiding this comment

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

its decided, nuke it.

public const string Patch = "PATCH";
public const string Post = "POST";
public const string Put = "PUT";
public const string Trace = "TRACE";
Copy link
Member

Choose a reason for hiding this comment

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

I'm for keeping these const. ReferenceEquals still works across assembly boundaries. Just don't mess up 😛 .

Copy link
Member

Choose a reason for hiding this comment

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

static readonly

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, constants would be nicer (e.g they could be used for attribute values) 👍

@mikaelm12
Copy link
Contributor Author

⬆️ 📅

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinchalet
Copy link
Contributor

a more optimized approach would be to have methods for each of the verbs that does a reference equality before doing the case insensitive matching.

What's the point of doing that manually? It's already something StringComparer does internally so you'll end up making the reference equality check twice :trollface:

@davidfowl
Copy link
Member

What's the point of doing that manually? It's already something StringComparer does internally so you'll end up making the reference equality check twice :trollface:

Good to know! https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/StringComparer.cs#L300

http://referencesource.microsoft.com/#mscorlib/system/stringcomparer.cs,285

We'll still do the work to use the same objects everywhere 😄

@kevinchalet
Copy link
Contributor

kevinchalet commented Sep 28, 2016

We'll still do the work to use the same objects everywhere 😄

TBH, I'm not sure to understand why (and how) having a static field would be better than a good old constant. I'm definitely not a perf' expert, but I'd expect the CLR to be smart enough to intern these constants to avoid instantiating new strings every time you use them in your code.

@khellang
Copy link
Contributor

I'm definitely not a perf' expert, but I'd expect the CLR to be smart enough to intern these constants to avoid instantiating new strings every time you use them in your code.

Because of interning, there's little to no difference between const and static readonly strings AFAIK.

Since the compiler will burn const values into all assemblies using them, it makes sense to use static readonly for values that might change at some point in the future, but I can't imagine any of the HTTP methods would change, ever. 😜

There's also a drawback to using static readonly; it can't be used in attributes 😢

@davidfowl
Copy link
Member

TBH, I'm not sure to understand why (and how) having a static field would be better than a good old constant. I'm definitely not a perf' expert, but I'd expect the CLR to be smart enough to intern these constants to avoid instantiating new strings every time you use them in your code.

Strings are deduped within single assembly boundaries. There's no guarantee that strings are "reference equal" across assembly boundaries. Using the same static readonly field guarantees that.

@halter73
Copy link
Member

There's no guarantee that strings are "reference equal" across assembly boundaries.

@davidfowl Did you read @khellang's comment?

Because of interning, there's little to no difference between const and static readonly strings AFAIK.

I've tested this, and this is true. const string are reference equal to literals/statics/etc... across assembly boundaries. That's why I was pushing for const earlier. I can't see the compiler/runtime team ever changing this considering how breaking it would be to do so.

@davidfowl
Copy link
Member

I've tested this, and this is true. const string are reference equal to literals/statics/etc... across assembly boundaries. That's why I was pushing for const earlier. I can't see the compiler/runtime team ever changing this considering how breaking it would be to do so.

I tested it as well and it does work. Still best practice is to avoid const leaking across assembly boundaries no matter how little difference it makes on current runtimes. Maybe it'll break when corert comes around 😉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants