Optimize server request .uri for pekko-http and akka-http #3566
Conversation
| case AkkaUri.Query.Cons(k, v, tail) => { | ||
| if (k.isEmpty) | ||
| run(tail, QuerySegment.Value(v) :: acc) | ||
| else if (v.isEmpty) |
There was a problem hiding this comment.
that's not necessarily correct - you'd have to check how Uris: ?a (just the value), ?a= and ?=b (is this even valid?) get parsed by both. There might be some edge cases where in one of those cases we actually have a key-value pair with an empty value, as opposed to an only-value query segment
There was a problem hiding this comment.
Good point, I think it's valid (https://github.com/apache/incubator-pekko-http/blob/6ccdc742747ba0ead83bc6e2b45554be412691e3/http-core/src/test/java/org/apache/pekko/http/javadsl/model/UriTest.java#L178), I'll test it.
There was a problem hiding this comment.
@adamw I checked it, and pekko-http parses "http://example.com?a=&=c&d to:
key="a", value=""key="", value="c"key=""d", value=""
And if we do sttp.model.Uri.unsafeParse(pekkoUri.toString), Tapir will construct:
KeyValue(k="a", v="")KeyValue(k="", v="c")Value(v="d")
Getting such a representation manually doesn't seem possible by traversing pekko's datatypes, because ?d is a Cons(k="d", v=""), just like it would be a ?d=.
What do we risk specifically by requiring this case to be a Value(v ="d") instead of a KeyValue(k="d", v="")?
There was a problem hiding this comment.
I guess we can then simply represent a= as a KeyValue, and the others as a Value? That should be close enough. If pekko doesn't differentiate here, we don't have to as well
There was a problem hiding this comment.
There's no difference between ?a and ?a= for Pekko, so we need to decide whether to treat them both as Value or a KeyValue with an empty value.
Fixes #3565