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

Missing documentation and weird usage #36

Open
uuf6429 opened this issue Apr 19, 2020 · 14 comments
Open

Missing documentation and weird usage #36

uuf6429 opened this issue Apr 19, 2020 · 14 comments

Comments

@uuf6429
Copy link
Contributor

uuf6429 commented Apr 19, 2020

I really like this project, however, not only is documentation lacking, but the examples seem broken/half-baked (eg TestServer.kt's APIPrincipal) and some usages are far from understandable.

For example, I just (literally) got a headache trying to understand how the StatusPages syntax works.

Let's give the original usage a look:

exception<Throwable> { cause ->      // easy, when Throwable is caught, InternalServerError is returned
    call.respond(HttpStatusCode.InternalServerError)
}

And the new usage:

exception<ProperException, Error>(HttpStatusCode.BadRequest) {
     // ^ when ProperException is thrown (what's a ProperException???), and... Error??? (parameter "B"... very useful..) then we have hard-coded status code?
    it.printStackTrace()
    Error(it.id, it.localizedMessage)  // and for some reason, return an Error
}

Similarly, the "basic" example in the readme also falls short in explaining anything and is hard to grasp:

    //bare minimum, just like Ktor but strongly typed
    get<StringParam, StringResponse> { params ->               // ok... get what exactly? where are we?
        respond(StringResponse(params.a))
    }

    route("inine").get<StringParam, StringResponse>(      // what's "inline"??
        info("String Param Endpoint", "This is a String Param Endpoint"), // A Route module that describes an endpoint, it is optional
        example = StringResponse("Hi")
    ) { params ->
        respond(StringResponse(params.a))   //  what's params and why do we use params.a???
    }

If writing proper examples and tests is too much effort, at least using better named symbols helps.

(to put some context, I spent half a day trying to convert two of my existing endpoints and spent most of the time digging in examples and internal source code trying to figure things out)

@Wicpar
Copy link
Collaborator

Wicpar commented Apr 19, 2020

The project has evolved and the documentation and examples haven't.
It would be nice to have some help writing the wiki, i have trouble structuring it into relevant/less relevant information for beginners.

Exception handling is not yet completely fleshed out, but got improved in 0.2-experimental:

throws(HttpStatusCode.BadRequest, CustomException::class) { ... // no example, just the exception handling
throws(HttpStatusCode.BadRequest, "example", CustomException::class) { ... // exception  handling with example, will respond example
throws(HttpStatusCode.BadRequest, "example", {ex: CustomException -> ex.toString()}) { ... // exception handling, will respond generated content

The usage would be clearer like this:

exception<TheExceptionToCatch, ReturnTypeToSerialize>(
HttpStatusCode.BadRequest // the response status code
) {
    it.printStackTrace() // do stuff like print stacktrace
    ReturnTypeToSerialize(it)  // return any type you want to respond (strings generally)
}

Status codes are hard coded per handler because OpenAPI needs to know statically what it needs to be, without the endpoint being called.

For the routes:

    get<ParameterType, ResponseType> { params ->       // get request on '/' because no path is specified
        respond(ResponseType(...))
    }

    route("someroute").get<ParameterType, ResponseType>(   //  get request on '/someroute'
        info("String Param Endpoint", "This is a String Param Endpoint"), // A Route module that describes an endpoint, it is optional
        example = ResponseType(...) // example for the OpenAPI data
    ) { params ->
        respond(ResponseType(...))
    }

routes can be written in a chained manner:

route("a").route("b")...

or

route("a") {
    route("b") {
        ...
    }
}

both are strictly the same and generate handlers that will prepend "/a/b" to get or post handlers.

Is this clearer ?

@uuf6429
Copy link
Contributor Author

uuf6429 commented Apr 19, 2020

Is this clearer ?

Yes, very much. Would be nice to have these sort of examples. If I manage to get some time, I'll try contributing as well.

@JavierPAYTEF
Copy link

Hi, to add a little to the other request, it would be nice to have a full example project, with all the verbs used.
I couldn't find examples using "post", or simple things like "get" example that doesn't receive any parameters but returns something.

One thing to mention is that the code appears to have no comments, so I have trouble understanding what I need to put in some of the parameters.

For example, if I see the following piece of code I don't know what is P, or R, I also don't know what I can put in RouteOpenAPIModule apart from info.
I'm switching between navigating the source in github and trying to guess what goes where based on the few examples.
If I had a comment saying "R is the request type, if you don't have input parameters set it to blah" or "@param modules You can use [info] here to add descriptions" it would be super useful.

@ContextDsl
inline fun <reified P : Any, reified R : Any> NormalOpenAPIRoute.get(
    vararg modules: RouteOpenAPIModule,
    example: R? = null,
    crossinline body: suspend OpenAPIPipelineResponseContext<R>.(P) -> Unit
) = route(HttpMethod.Get, modules, example, body)

@Wicpar
Copy link
Collaborator

Wicpar commented Jun 29, 2020

I agree, If you do a pull request with documentation comments i'll accept it.
It might take while until this is worked on on my side if not.

@uuf6429
Copy link
Contributor Author

uuf6429 commented Jun 29, 2020

To be fair, I think that using P, T etc is a bad practice coming from Kotlin. Most Kotlin generic functionality does this.
It would have been nice to adopt a standard such as TSomething's, eg, TRequest, TParams or:

public fun <TValue> listOf(vararg elements: TValue): List<TValue> =
     ...

@JavierPAYTEF
Copy link

It would definitely help to have those generics have a better name like TParams, TRequest, TResponse.

In the Kotlin example you sent "elements" kind of explains what T is, so we can give it pass, since it would be redundant to have "elements: TElement".

But, for the example I sent, your idea is good because no parameter explains what R or P are, the only names in the method are "modules", "example" and "body", and none of those are "response" or "parameters" (I assume the P is for Parameters?).
So better names would make it more understandable for sure without extensive documentation.

@Wicpar I can try to add some comments and improve the names if you are open to it. Maybe I should start with the classes more likely to be used by users. Which packages do you think would be good to start on?

@Wicpar
Copy link
Collaborator

Wicpar commented Jun 30, 2020

Yes, TSomething is a way better pattern than the first letter capitalised, i'll start using it.

@JavierPAYTEF the best package to start on is com.papsign.ktor.openapigen.route.path

@JavierPAYTEF
Copy link

Hey I'm currently writting the docs and changing the names but I'm going crazy with a problem I can't find the reason for. The body and response for all my services are empty.
I managed to track it to RequestHandlerModule when it does provider.ofType<BodyParser>() it always returns null.
I'm not sure why the BodyParser module is empty. Do I need to register the BodyParser myself someway?

This is an example of the data clases not appearing:

data class PinPadStatusReq(
    var language: String = "es", // es
    var pinpad: String = "*" // *
)

data class PinPadStatusResp(
    var datacenterMessage: String = "Conexión estable",
    var datacenterPingMS: Int = 140,
    var pinpads: List<Pinpad> = listOf(Pinpad())
) {
    data class Pinpad(
        var default: Boolean = true,
        var serialNumber: String = "123456789",
        var status: String = "#connected"
    )
}

This is my configuration:

val api = install(OpenAPIGen) {
	oaConfig.installIn(this)
}
install(ContentNegotiation) {
	/*jackson {
		setSerializationInclusion(JsonInclude.Include.NON_NULL)
		enable(SerializationFeature.INDENT_OUTPUT) // Pretty Prints the JSON
	}*/
	jackson {
		enable(
			DeserializationFeature.WRAP_EXCEPTIONS,
			DeserializationFeature.USE_BIG_INTEGER_FOR_INTS,
			DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS
		)

		enable(SerializationFeature.WRAP_EXCEPTIONS, SerializationFeature.INDENT_OUTPUT)

		setSerializationInclusion(JsonInclude.Include.NON_NULL)

		setDefaultPrettyPrinter(DefaultPrettyPrinter().apply {
			indentArraysWith(DefaultPrettyPrinter.FixedSpaceIndenter.instance)
			indentObjectsWith(DefaultIndenter("  ", "\n"))
		})

		//registerModule(JavaTimeModule())
	}
}
install(CallLogging) {
	level = Level.DEBUG
}
install(StatusPages) {
	withAPI(api) {
		exception<Exception, ErrorResponse>(HttpStatusCode.InternalServerError) { exception ->
			ResponseBuilder.error(exception)
		}
		status(HttpStatusCode.NotFound) {
			call.respond(
				HttpStatusCode.NotFound,
				ResponseBuilder.error("notFound", "Requested page was not found")
			)
		}
	}
}
routing{
	get("/openapi.json") {
		call.respond(application.openAPIGen.api.serialize())
	}
	get("/swagger") {
		call.respondRedirect("/swagger-ui/index.html?url=/openapi.json", true)
	}
}
apiRouting {
	route("/pinpad/status") {
		post<Unit, PinPadStatusResp, PinPadStatusReq>(
			info("PIN/PAD status"),
			exampleResponse = PinPadStatusResp(),
			exampleRequest = PinPadStatusReq()
		) { _, body ->
			respond(PinPadStatusResp())
		}
	}
}

This is what the json looks like:

json

@JavierPAYTEF
Copy link

JavierPAYTEF commented Jul 1, 2020

I made it work somehow, but had to change part of the configuration, I'm guessing this is not done because I'm compiling in Android and it's not detecting something. If I do the following it works:

val api = install(OpenAPIGen) {
	// ... the default config here ...
	addModules(
		DefaultCollectionSchemaProvider,
		DefaultEnumSchemaProvider,
		DefaultMapSchemaProvider,
		DefaultObjectSchemaProvider,
		DefaultPrimitiveSchemaProvider,
		DefaultSetSchemaProvider,
		NothingSchemaProvider
	)
}
FinalSchemaBuilderProvider.onInit(api)
KtorContentProvider.onInit(api)

@Wicpar
Copy link
Collaborator

Wicpar commented Jul 5, 2020

@JavierPAYTEF sorry for the late response.
The library uses reflections to detect the default modules. I didn't think to test with Android if this worked. It seems that it doesn't.

@JavierPAYTEF
Copy link

Hi @Wicpar I created a pull request that adds a couple of the changes we talked about in here.
Please let me know if I'm on the right track, you might want to edit some of the texts, please let me know.
The pull request shouldn't have any functional changes compared to the current version, it should only be name changes and comments.

@mattrussell-sonocent
Copy link

I couldn't find examples using "post", or simple things like "get" example that doesn't receive any parameters but returns something.

This may not be the right place to ask, but how do you do a get that doesn't take any parameters?

@Wicpar
Copy link
Collaborator

Wicpar commented Nov 21, 2023

You can useUnit

@mattrussell-sonocent
Copy link

You can useUnit

👍 Thanks

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

No branches or pull requests

4 participants