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

Features headers to remove not working #120

Closed
pietrini opened this issue Feb 20, 2020 · 6 comments · Fixed by #121
Closed

Features headers to remove not working #120

pietrini opened this issue Feb 20, 2020 · 6 comments · Fixed by #121
Labels
bug Something isn't working
Milestone

Comments

@pietrini
Copy link

Using the new 1.4.0 version and the previous one 1.3.0, headers to remove function isn't working.

This is the output of my successful generateSwagger task:

> Task :app:generateSwagger
Successfully read version from Swagger Spec file: 1.1.0
####################
Yelp Swagger Codegen
####################
Platform 	 kotlin-coroutines
Package 	 com.my.super.project
specName 	 [ DEFAULT ] defaultname
specVers 	 1.1.0
input 		 /home/projects/app/swagger.json
output 		 /home/projects/app/src/main/java
groupId 	 com.my.super.project
artifactId 	 com.my.super.project
features 	 X-HTTP-Method-Override, X-Operation-ID, Content-Type

BUILD SUCCESSFUL in 1s
1 actionable task: 1 executed
10:09:41 PM: Task execution finished 'generateSwagger'.

And this is my gradle configuration:

generateSwagger {
    platform = "kotlin-coroutines"
    packageName = "com.my.super.project"
    outputDir = file("./src/main/java/")
    features {
        headersToRemove = ["X-HTTP-Method-Override", "X-Operation-ID", "Content-Type"]
    }
    inputFile = file("../swagger/destination/PointsOfInterest_v1_Swagger_specification.json")
}

And this is the output of my Api:

@Headers(
        "X-Operation-ID: getPointOfInterest",
      "Content-Type: application/json"
    )
    @GET("api/{poisId}")
    suspend fun getPointOfInterest(
        @retrofit2.http.Path("poisId") poisId: String
    ): Map<String, Any?>

By the awesome project and thx for the help !

cortinico added a commit that referenced this issue Feb 20, 2020
Make sure we actually filter out also top level headers
and not only parameter headers with the `headersToRemove` feature.

Fixes #120
@cortinico cortinico added the bug Something isn't working label Feb 20, 2020
@cortinico
Copy link
Collaborator

Hi @pietrini thanks for reporting,

This is not actually a regression but rather a know issue.
Both "X-Operation-ID" and "Content-Type" are treated specially as they're top level header for the Retrofit function. headersToRemove currently works only for parameter headers, like the "X-HTTP-Method-Override".

Anyway it makes sense to fix this and make sure that headersToRemove actually filters out also those two special headers if needed.

Here the fix #121

By the awesome project and thx for the help !

<3

@cortinico
Copy link
Collaborator

@macisamuele @martinbonnin do we want to release this in a potential 1.4.1 as a bugfix? Or we let this go inside 2.x?

@martinbonnin
Copy link
Contributor

I'd go for a 1.4.1 release since 2.x will require more effort to integrate. But I'm not the one doing the releases so that's the easy choice :)

@macisamuele
Copy link
Collaborator

I agree with @martinbonnin, I'm not the one releasing so the final call is on @cortinico :)
Something worth mentioning is that releases are free, so honestly if there is a question like "should we release?" then the usual answer would be yes.
The only exception that I would consider is in case there are incomplete/bugged features that are in master and should not be released yet (ie. we're working on lazy configs and we found major issues or we're working on polymorphism and generated code is not good).

@cortinico cortinico added this to the 1.4.1 milestone Feb 21, 2020
@cortinico
Copy link
Collaborator

So @pietrini version 1.4.1 is out with this fix.

@pietrini
Copy link
Author

Impressive thx !!!!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants