Cookie handling #104

kennethjor · 2021-11-16T07:31:50Z

Implemented @CookieParam support for String and Netty's Cookie type.

src/main/java/io/cdap/http/internal/HttpResourceModel.java

kennethjor · 2021-11-16T16:33:35Z

Updated the PR to support Netty's Cookie type as well.

For reference, I did try and add support for javax.ws.rs.core.Cookie, but got the following exception during my testing:

01:15:41.488 [test-worker-thread-0] ERROR io.cdap.http.internal.HttpDispatcher - Exception caught in channel processing.
io.cdap.http.internal.HandlerException: Error in executing request: GET /test/v1/jaxCookieParam
	at io.cdap.http.internal.HttpResourceModel.handle(HttpResourceModel.java:161) ~[classes/:na]
	at io.cdap.http.internal.HttpResourceHandler.getDestinationMethod(HttpResourceHandler.java:292) ~[classes/:na]
	at io.cdap.http.internal.RequestRouter.prepareHandleMethod(RequestRouter.java:149) ~[classes/:na]
	at io.cdap.http.internal.RequestRouter.channelRead(RequestRouter.java:88) ~[classes/:na]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.ChannelInboundHandlerAdapter.channelRead(ChannelInboundHandlerAdapter.java:86) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.handler.codec.http.HttpServerKeepAliveHandler.channelRead(HttpServerKeepAliveHandler.java:64) ~[netty-codec-http-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.ChannelInboundHandlerAdapter.channelRead(ChannelInboundHandlerAdapter.java:86) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:102) ~[netty-codec-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.handler.codec.MessageToMessageCodec.channelRead(MessageToMessageCodec.java:111) ~[netty-codec-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:438) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:310) ~[netty-codec-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:284) ~[netty-codec-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:253) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:935) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:134) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:645) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:580) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:497) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:459) ~[netty-transport-4.1.16.Final.jar:4.1.16.Final]
	at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858) ~[netty-common-4.1.16.Final.jar:4.1.16.Final]
	at java.base/java.lang.Thread.run(Thread.java:829) ~[na:na]
Caused by: java.lang.ExceptionInInitializerError: null
	at io.cdap.http.internal.ParamConvertUtils$4.convert(ParamConvertUtils.java:125) ~[classes/:na]
	at io.cdap.http.internal.ParamConvertUtils$4.convert(ParamConvertUtils.java:121) ~[classes/:na]
	at io.cdap.http.internal.HttpResourceModel$ParameterInfo.convert(HttpResourceModel.java:326) ~[classes/:na]
	at io.cdap.http.internal.HttpResourceModel.getCookieParamValue(HttpResourceModel.java:215) ~[classes/:na]
	at io.cdap.http.internal.HttpResourceModel.handle(HttpResourceModel.java:148) ~[classes/:na]
	... 38 common frames omitted
Caused by: java.lang.RuntimeException: java.lang.ClassNotFoundException: org.glassfish.jersey.internal.RuntimeDelegateImpl
	at javax.ws.rs.ext.RuntimeDelegate.findDelegate(RuntimeDelegate.java:152) ~[javax.ws.rs-api-2.0.jar:2.0]
	at javax.ws.rs.ext.RuntimeDelegate.getInstance(RuntimeDelegate.java:120) ~[javax.ws.rs-api-2.0.jar:2.0]
	at javax.ws.rs.core.Cookie.<clinit>(Cookie.java:61) ~[javax.ws.rs-api-2.0.jar:2.0]
	... 43 common frames omitted
Caused by: java.lang.ClassNotFoundException: org.glassfish.jersey.internal.RuntimeDelegateImpl
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581) ~[na:na]
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) ~[na:na]
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) ~[na:na]
	at java.base/java.lang.Class.forName0(Native Method) ~[na:na]
	at java.base/java.lang.Class.forName(Class.java:315) ~[na:na]
	at javax.ws.rs.ext.FactoryFinder.newInstance(FactoryFinder.java:114) ~[javax.ws.rs-api-2.0.jar:2.0]
	at javax.ws.rs.ext.FactoryFinder.find(FactoryFinder.java:207) ~[javax.ws.rs-api-2.0.jar:2.0]
	at javax.ws.rs.ext.RuntimeDelegate.findDelegate(RuntimeDelegate.java:135) ~[javax.ws.rs-api-2.0.jar:2.0]
	... 45 common frames omitted

If you know how to fix this, I'll gladly re-add support for JAX-RS cookies as well.

kennethjor · 2021-11-26T05:36:11Z

@chtyim Ping :)

kennethjor · 2022-01-19T02:49:19Z

@chtyim Another gentle reminder.

kennethjor · 2022-06-12T05:51:54Z

@chtyim Any change of getting this merged?

albertshau

one actual comment, the rest are just about code style.

Thanks for the contribution!

albertshau · 2022-06-14T16:17:55Z

src/main/java/io/cdap/http/internal/HttpResourceModel.java

@@ -125,6 +130,8 @@ public HttpMethodInfo handle(HttpRequest request,
      if (httpMethods.contains(request.method())) {
        //Setup args for reflection call
        Object [] args = new Object[paramsInfo.size()];
+        // Parse cookies
+        Map<String, Cookie> cookies = cookieParser.parseCookies(request);


One of the problems with parsing here is that an invalid cookie will cause the request to fail, even if the method doesn't care about it. It could potentially cause backwards incompatibilities, as somebody could upgrade this library and then see their requests failing due to some headers they never looked at before.

instead of parsing all the cookies here, it would be better to only lookup the relevant header in the getCookieParamValue() method and decode it there.

I have moved it to the method so it will only be run on-demand. It does mean though that every cookie will result in parsing all the cookies. Is there a way to cache this? All the members on the HttpResourceModel are final and set in the constructor, so I wasn't sure if it would be ok to add a cookie parsing cache there, which is the approach I'd normally take.

src/test/java/io/cdap/http/internal/CookieParserTest.java

src/test/java/io/cdap/http/TestHandler.java

src/test/java/io/cdap/http/HttpServerTest.java

Implemented String-based cookie handling.

ad4911b

chtyim reviewed Nov 16, 2021

View reviewed changes

src/main/java/io/cdap/http/internal/HttpResourceModel.java Outdated Show resolved Hide resolved

Added support for DefaultValue on cookies.

5a55b5c

kennethjor requested a review from chtyim November 16, 2021 11:45

kennethjor added 3 commits November 17, 2021 01:24

Added support for Netty Cookie objects.

d4005f3

Readme.

425b08e

Simplified cookie testing.

eb68a86

kennethjor changed the title ~~String-based cookie handling~~ Cookie handling Nov 16, 2021

albertshau reviewed Jun 14, 2022

View reviewed changes

kennethjor added 3 commits November 5, 2022 16:25

Merge branch 'cdapio:develop' into kennethjor/cookie-param

0e9f971

Code style.

a5662a4

Moved cookie parsing to getCookieParamValue.

a0418c9

kennethjor requested review from albertshau and chtyim and removed request for chtyim and albertshau November 5, 2022 07:41

kennethjor requested review from albertshau and removed request for chtyim March 2, 2023 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cookie handling #104

Cookie handling #104

kennethjor commented Nov 16, 2021 •

edited

Loading

kennethjor commented Nov 16, 2021 •

edited

Loading

kennethjor commented Nov 26, 2021

kennethjor commented Jan 19, 2022

kennethjor commented Jun 12, 2022

albertshau left a comment

albertshau Jun 14, 2022

kennethjor Nov 5, 2022

Cookie handling #104

Are you sure you want to change the base?

Cookie handling #104

Conversation

kennethjor commented Nov 16, 2021 • edited Loading

kennethjor commented Nov 16, 2021 • edited Loading

kennethjor commented Nov 26, 2021

kennethjor commented Jan 19, 2022

kennethjor commented Jun 12, 2022

albertshau left a comment

Choose a reason for hiding this comment

albertshau Jun 14, 2022

Choose a reason for hiding this comment

kennethjor Nov 5, 2022

Choose a reason for hiding this comment

kennethjor commented Nov 16, 2021 •

edited

Loading

kennethjor commented Nov 16, 2021 •

edited

Loading