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

Refit doesnt allow constant variable from another class ? #263

Closed
hkaraoglu opened this issue Sep 16, 2016 · 21 comments
Closed

Refit doesnt allow constant variable from another class ? #263

hkaraoglu opened this issue Sep 16, 2016 · 21 comments
Labels
semantic-analysis Bug reports/change requests that would require semantic analysis in the stub generator up-for-grabs

Comments

@hkaraoglu
Copy link

How to define a constant variable for path "index.php?route=mobile" ? I defined in another class a constant variable.

Config.cs

public const  String SERVICE_PATH = "/index.php?route=mobile";

APIService.cs

 [Post(Config.SERVICE_PATH+"/login")]
        Task<string> login([Body(BodySerializationMethod.UrlEncoded)] Dictionary<string, string> userInfo);

But refit throws an error like this :

System.NotImplementedException: Either this method has no Refit HTTP method attribute or you've used something other than a string literal for the 'path' argument.

Full part of my service interface :

public interface APIService
{
        [Post("/index.php?route=mobile/login")]
        Task<string> login([Body(BodySerializationMethod.UrlEncoded)] Dictionary<string, string> userInfo);

        [Post("/index.php?route=mobile/account/register")]
        Task<string> register([Body(BodySerializationMethod.UrlEncoded)] Dictionary<string, string> userInfo);

        [Get("/index.php?route=mobile/account/forgotPassword&email={email}")]
        Task<string> forgotPassword(String email);
}
@hkaraoglu hkaraoglu changed the title How to define a variable in interface ? Refit doesnt allow constant variable from another class ? Sep 16, 2016
@ahmedalejo
Copy link

try using the following

public interface APIService
{
        [Post("/index.php?route=mobile%2Flogin")]
        Task<string> login([Body(BodySerializationMethod.UrlEncoded)] Dictionary<string, string> userInfo);

        [Post("/index.php?route=mobile%2Faccount%2Fregister")]
        Task<string> register([Body(BodySerializationMethod.UrlEncoded)] Dictionary<string, string> userInfo);

        [Get("/index.php?route=mobile%2Faccount%2FforgotPassword")]
        Task<string> forgotPassword(String email);
}

@skoon
Copy link

skoon commented Sep 16, 2016

This is a weird one. Shouldn't the const get inlined at compile time? I'm assuming you get the same error if you redefine the const as a static readonly string?
public static readonly String SERVICE_PATH = "/index.php?route=mobile";

@ahmedalejo
Copy link

ahmedalejo commented Sep 16, 2016

@skoon the problem isn´t with the const string but what is in it.
there are multiple (unescaped) path separators after ?

what a Refit isn´t handling well is the
?route=mobile/account/register

and

public static readonly String SERVICE_PATH = "/index.php?route=mobile";

is definately not allowed as reference in Attribute annotations but public const String SERVICE_PATH = "/index.php?route=mobile" is.

@ahmedalejo
Copy link

ahmedalejo commented Sep 16, 2016

String != string

@Cheesebaron correct me if i´m wrong
aren´t the following statement correct?

String a = "a";
string b = "b";
var c = a + b;

a.Should().BeOfType<string>();
a.Should().BeOfType<String>();
b.Should().BeOfType<string>();
b.Should().BeOfType<String>();
c.Should().BeOfType<string>();
c.Should().BeOfType<String>();
//i.e
(a is String).Should().BeTrue();
(a is string).Should().BeTrue();
(b is String).Should().BeTrue();
(b is string).Should().BeTrue();
(c is String).Should().BeTrue();
(c is string).Should().BeTrue();

@Cheesebaron
Copy link
Contributor

@ahmedalejo you are right, I didn't think it through :)

As for the issue, why is it that the paths are not valid? Is it a limitation of Refit? Because Uri seems to support it just fine?

var derp = new Uri("http://example.org/index.php?route=mobile/account/register");

derp.PathAndQuery
"/index.php?route=mobile/account/register"

derp.Query
"?route=mobile/account/register"

But I would guess Refit does some parsing of the URL before making it into something passed to the HttpClient. This logic does not understand the unescaped /.

@ahmedalejo
Copy link

@Cheesebaron you are right, as well :)

it seems like a Refit limitation to handle / after a query parameter. But http://example.org/index.php?route=mobile/account/register is a valid Uri and Url.

as the following link from Gmail to this issue implies.

https://www.google.com/url?q=https://github.com/paulcbetts/refit/issues/263&source=gmail

i trimmed out some personal info from the Url but you get the point.

@hkaraoglu
Copy link
Author

hkaraoglu commented Sep 16, 2016

It works if I write directly into Post or Get annotations. But when I want to write as a variable it throws the above error.

@skoon
Copy link

skoon commented Sep 16, 2016

Yeah, the ReFit examples show unescaped slashes and I've used unescaped slashes when I've used the library in projects.

Which is why I wondered if changing it to static readonly had the same effect. The main difference being that static vars are looked up at runtime while const values are inlined where they are used at compile time.

So it could be that there is something going on with the const value NOT getting inlined into the attribute definition at compile time. Which would be weird.

@skoon
Copy link

skoon commented Sep 16, 2016

Also this happens at compile time, not runtime. So a static readonly won't work at all since attributes don't allow them

🤦

@bennor
Copy link
Contributor

bennor commented Sep 16, 2016

Since 2.0, Refit generates implementations of the interfaces at build time (mostly for iOS support, IIRC). The reason constants aren't inlined is that the generation is done my interrogating the abstract syntax tree for the interface.

We use a few assumptions to verify the method definitions have the correct attributes, and supporting constants in the URL templates would make things difficult (the constant may be defined in another file, for example) which would have meant a significant rewrite of the generator code. (I believe it would need to change to using semantic analysis instead.)

Someone is welcome to look into it and submit a PR, but it could be a lot of work for a fairly uncommon use case. (In a couple of years since v2, this is the first time I think it's been raised as an issue).

@clairernovotny
Copy link
Member

@bennor it's not just for iOS that we do compile-time code gen, it's also for the WinRT/UWP platforms as well.

@ahmedalejo
Copy link

ahmedalejo commented Sep 23, 2016

It compiles for all platforms

Generated in the obj folder as a ".g.cs" file
On Fri, Sep 23, 2016 at 9:10 AM Oren Novotny notifications@github.com
wrote:

@bennor https://github.com/bennor it's not just for iOS that we do
compile-time code gen, it's also for the WinRT/UWP platforms as well.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGr-6vfqcM4uf_pTH7HLclLR9BhYWlqQks5qs8HFgaJpZM4J_EF4
.

@bennor
Copy link
Contributor

bennor commented Sep 23, 2016

Yeah, I know how it works 😂

I was referring to why the change was made from runtime method interception in 2.0. Was originally for iOS but then we added support for WinRT.

@jamiehowarth0
Copy link
Member

jamiehowarth0 commented Jul 18, 2019

I'm happy calling it that we shouldn't support this, constants aren't supported in default variable values in C# signatures, up to and including version 8 (you can't specify a default string value as string.Empty), I don't see why they should be supported in parameter values.

@bennor
Copy link
Contributor

bennor commented Jul 23, 2019

@benjaminhowarth1 The original request was for constant values to work in attribute parameters, which is something .NET supports.

The problem is more about how we generate the code from the interfaces. There are a whole class of issues that relate to the fact that we do syntax analysis on the project only, as opposed to semantic analysis factoring in the whole solution. Changing over would be a big change and may have some serious implications for performance.

@jamiehowarth0
Copy link
Member

Ah, I didn't realise, thanks! Interface inheritance also has issues around this, right?

@bennor
Copy link
Contributor

bennor commented Jul 23, 2019

Yeah. There are a few others too. I've tagged them with the semantic-analysis label until we figure out what to do with them.

@lobster2012-user
Copy link

lobster2012-user commented Sep 15, 2019

Hi!
The builder works with the attribute. I do not see the need for the latter condition, commented it out, now you can use everything that is allowed in the attributes.
Perhaps my English is not very good, I still do not understand why this condition is necessary.
Add at least the ability to set a variable in .csproj to use expressions if necessary.

Why do we need these constants? At a minimum, to avoid code duplication (In my case)

public bool HasRefitHttpMethodAttribute(MethodDeclarationSyntax method)
        {
            // We could also verify that the single argument is a string, 
            // but what if somebody is dumb and uses a constant?
            // Could be turtles all the way down.
            return method.AttributeLists.SelectMany(a => a.Attributes)
                         .Any(a => HttpMethodAttributeNames.Contains(a.Name.ToString().Split('.').Last()) &&
                                   a.ArgumentList.Arguments.Count == 1 //&&
                                  //(
                                  //a.ArgumentList.Arguments[0].Expression.Kind()
                                  //== SyntaxKind.StringLiteralExpression
                                  //)
                                  );
        }
public const string RestService_ConstString_Path = "/const";

        public interface IRestService_ConstString_Api
        {
            [Get(RestService_ConstString_Path + "/a")]
            Task Get();
        }

       

        [Fact]
        public async Task RestService_ConstString_Test()
        {
            var mockHttp = new MockHttpMessageHandler();

            var settings = new RefitSettings
            {
                HttpMessageHandlerFactory = () => mockHttp
            };

            mockHttp.Expect(HttpMethod.Get, "http://httpbin.org" +
                RestService_ConstString_Path + "/a")
                    .Respond(HttpStatusCode.OK);

            var fixture = RestService.For<IRestService_ConstString_Api>(
                "http://httpbin.org/", settings);


            await fixture.Get();

            mockHttp.VerifyNoOutstandingExpectation();
        }

@clairernovotny
Copy link
Member

Should be implemented in #1029

@clairernovotny
Copy link
Member

Please try v6.0-preview.84 and file bugs as you come across them.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semantic-analysis Bug reports/change requests that would require semantic analysis in the stub generator up-for-grabs
Projects
None yet
Development

No branches or pull requests

8 participants