JonathanGiles / TeenyHttpd

TeenyHttpd is a HTTP server written in Java.
MIT License
17 stars 3 forks source link

TeenyApplication #2

Closed alex-cova closed 9 months ago

alex-cova commented 9 months ago

This pull-request includes, a teeny json parser/writer, winter a utility class to create teeny web-apps and minor changes compared to the last PR (uff I spent a lot of time trying to create a JSON reader that supports nested objects, more hard that I thought)

alex-cova commented 9 months ago

Would be awesome if you can add https://github.com/JetBrains/java-annotations to the project for better null-checks and better kotlin integration

JonathanGiles commented 9 months ago

Would be awesome if you can add https://github.com/JetBrains/java-annotations to the project for better null-checks and better kotlin integration

null checks feel like a danger field in Java, but I am very sympathetic to their value when combined with good tooling. So far there is no specific tooling in this repo (e.g. CheckStyle, SpotBugs, Jacoco, Spotless, etc). I think we can have a separate issue / PR to discuss this, because - why not JSpecify or some other library instead? I am not fond of kotlin integration though :-)

alex-cova commented 9 months ago

Would be awesome if you can add https://github.com/JetBrains/java-annotations to the project for better null-checks and better kotlin integration

null checks feel like a danger field in Java, but I am very sympathetic to their value when combined with good tooling. So far there is no specific tooling in this repo (e.g. CheckStyle, SpotBugs, Jacoco, Spotless, etc). I think we can have a separate issue / PR to discuss this, because - why not JSpecify or some other library instead? I am not fond of kotlin integration though :-)

Just suggested this annotations because works out-of-the box with IntelliJ https://www.jetbrains.com/help/idea/annotating-source-code.html#external-annotations no worries, I adapt to what is required/provided, thank you for the feedback I'll close the PR to make the corresponding changes, let's go!

JonathanGiles commented 9 months ago

You don't need to close this pr- just submit updates to it

alex-cova commented 9 months ago

Quick feedback from my phone. Will take a better look tomorrow, but it is looking great! Btw, do you have a Twitter handle?

Yes, this is my twitter user: invokespecial

JonathanGiles commented 9 months ago

Sorry, I've been unwell the last two days so I haven't managed to get to this. I will try later today. One thought: it would be cool if the controller can support SSE paths too, seeing as I just added SSE support to TeenyHttpd.

Another thing, can you please update the PR title? Thanks!

alex-cova commented 9 months ago

Sorry, I've been unwell the last two days so I haven't managed to get to this. I will try later today. One thought: it would be cool if the controller can support SSE paths too, seeing as I just added SSE support to TeenyHttpd.

Another thing, can you please update the PR title? Thanks!

No worries, I hope You get well soon, next commit I plan to add 'filters/middleWares` (for Basic/JWT Authentication) and CORS configuration (also a middleware/filter) something I think is missing on TeenyHttpd.

@Middleware(order = 1)
class CorsFilter implements TeenyMiddleware {

Request handle(Request request, TeenyMiddleware next) {
       request = next.handle(request);

       request.addHeader("Access-Control-Allow-Origin", "*");

       return request
   }
}

is it good?

JonathanGiles commented 9 months ago

Sorry, I've been unwell the last two days so I haven't managed to get to this. I will try later today. One thought: it would be cool if the controller can support SSE paths too, seeing as I just added SSE support to TeenyHttpd. Another thing, can you please update the PR title? Thanks!

No worries, I hope You get well soon, next commit I plan to add 'filters/middleWares` (for Basic/JWT Authentication) and CORS configuration (also a middleware/filter) something I think is missing on TeenyHttpd.

@Middleware(order = 1)
class CorsFilter implements TeenyMiddleware {

Request handle(Request request, TeenyMiddleware next) {
       request = next.handle(request);

       request.addHeader("Access-Control-Allow-Origin", "*");

       return request
   }
}

is it good?

Yep - let's get this first PR merged and then we can start adding more features for auth and CORS to both layers.

JonathanGiles commented 9 months ago

Here's a list of things we've discussed - let me know if you want to do them in this PR or a later one:

alex-cova commented 9 months ago
  • SSE support in TeenyApplication

Implemented in the last commit

as simple as this:

 @ServerEvent("/messages")
  public ServerSentEventHandler chatMessages() {
      return ServerSentEventHandler.create();
   }
@Post("/message")
 public void message(@QueryParam("message") String message,
             @EventHandler("chatMessages") ServerSentEventHandler handler) {

        if (message.isEmpty()) {
            return;
        }

        handler.sendMessage(message);
    }
Screenshot 2024-02-17 at 7 36 02 p m

I did some changes to the chat.

We will need to add a ContextId to ServerEvent in case of having two methods named chatMessages annotated with ServerEvent but for now I think is ok.


  • Improvements to the Response API to have convenience APIs to create responses.

I have some questions and suggestions about this


  • Keeping perf up in the Content-Length header case (I'm planning to work on some perf benchmarks after this PR is done)


  • Updating the readme with the TeenyApplication details (I can write this later if you want)

Yes please


  • Adding auth / CORS middleware support

Planned but not in this PR


  • Adding addHeader support into Response / TypedResponse


  • Having TypedResponse use the same header handling code internally


  • Runtime annotation scanning for finding controllers and message converters

Already in the oven, but not in this PR

Observations

(Correct me if I'm wrong please)

I didn't added a StatusCode.asTypedResponse(Class<T> clazz, T Body) method specifying the body is enough to infer the type.

public <T> TypedResponse<T> asTypedResponse(T body) {
        return new TypedResponseImpl<>(this, body);
    }
JonathanGiles commented 9 months ago

Overall this PR is looking really good. I am happy to merge when you are happy. Let me know. We can smooth out the rough edges of everything in future PRs. Really cool contribution - thanks!

alex-cova commented 9 months ago

Overall this PR is looking really good. I am happy to merge when you are happy. Let me know. We can smooth out the rough edges of everything in future PRs. Really cool contribution - thanks!

Glad You liked it, looking forward to contribute even more, and improve more. I'm open to suggestions. I really enjoyed this PR, let's merge it