HttpCookie and its tests #3927

domaspoliakas · 2024-05-21T21:50:03Z

Not a very strict implementation of the RFCs, but I find that it largely matches how the JVM behaves. I tried copying from netty at first (as netty is also apache 2.0), but ultimately found that implementing this was more about matching the quirks of the JVM rather the RFCs, so I ended up implementing something from scratch.

I'll add some more tests, but given that this is my first PR to the project I wanted to make sure that the PR is in order. As such let me know if there's anything else needed here!

domaspoliakas · 2024-05-21T21:55:15Z

I have signed the CLA

LeeTibbert · 2024-05-23T03:26:43Z

javalib/src/main/scala/java/net/HttpCookie.scala

+    )
+  }
+
+  override def equals(obj: Any): Boolean = {


A style point. I recommend moving the override of hashCode below
to be either just before or just after the override of equals().
That will make it easier for people reading the code who go on alert
when they see equals being overridden to convince themselves
that hashCode is properly overridden in co-ordination.

As long as one thing is moving, I suggest moving toString() to
the same block so that overrides are together and easier to find.

Late at night now. By the light of day I will try to post a URL
to the current "Best Practice" for an equals method.

It helped me last week when I had to write an equals for
fileKeys. Ultimately, because of some wierdness with fileKey,
I could not follow it exactly, but at least I knew what I was deviating from and why.

Moved things around and implemented a new equals as per the advice you linked

LeeTibbert · 2024-05-23T03:29:02Z

javalib/src/main/scala/java/net/HttpCookie.scala

+  def getValue(): String = this._value
+  def getVersion(): Int = this._version
+  def hasExpired(): Boolean = this._maxAge == 0
+  override def hashCode(): Int = toString().hashCode()


This works and balances equals.

There is a recommendation that I have never quite understood as to
using .## instead of hashCode when defining a hashCode method.
I believe that String (and most other objects) have such. I believe
that .## handles some special cases.

There a are a number of override hashCode implementations
in the javalib code base, so it is hard to point out one which
is 'Best (or at least current) Practice'. Answering the question of
'why is that one better (or gives a better distribution)' could take
a semester course in Maths.

The one in java.util.Arrays gives a basic structure. It comes from
Scala.js, where folks are knowledgeable, experienced, and super strict.

A non array body would look something like the
example from nio/file/attribute/PosixFileAttributeViewImpl.scala

override def hashCode(): Int = { var res = 1 res = 31 * res + deviceId.## res = 31 * res + inodeNumber.## res }

The idea is a field-by-field comparison by accumulating and shifting the intermediate results. If the fields can be null, one must handle that (long discussion).

If I live a Good Life, Someday I may be saved from having to write
equals/hashcode pairs.

The JVM seems to include the name, value, domain and path into the hashcode, so I changed mine to do so as well, using the formula you gave there.

LeeTibbert · 2024-05-23T04:08:49Z

unit-tests/shared/src/test/scala/org/scalanative/testsuite/javalib/net/HttpCookieTest.scala

+    val cookie = out.get(0)
+    assertEquals("potato", cookie.getName())
+    assertEquals("tomato", cookie.getValue())
+    assertEquals(null, cookie.getComment())


A comment based on my personal style. Authors have wide latitude
to follow almost any correct style.

When writing tests, I like to take a few moments to think about
how a person running the test might figure out what went wrong and
where.

When there is only one assert in a Test, that is not a problem.
When there are 20 or 40 or so (some of the regex code), it can
mean many hours of tedious bi-section search.

assertEquals and many of its kin have a three argument form,
where the first argument is a message. It takes a developer
minutes of time to place a message, but it can save a debugger hours.
Usually when debugging, time is of the essence: thing are not good and are probably going to get worse.

Here one can probably figure out the failure point if one of the
first two fails first two. I suspect that line 20 and kin would
cause some puzzlement.

A quick search shows unit-tests/shared/src/test/require-scala3-jdk12/org/scalanative/testsuite/javalib/util/stream/CollectorsTestOnJDK12.scala following that practice.

Now meaningful messages are an ideal. In some cases where I
have had to figure out which of 5 or more assertions (especially
the dreaded assertTrue & assertFalse, which do not give values)
are failing and have less than zero time to do it, I fall back to
numbering the assertions "a1", "a2", etc. This gives me a brainless
way to distinguish them.

Yes, I hang my head that I should be taking the time to figure out a better message. That means figuring out what all sorts of quirky code
is supposed to be doing, where that code has nothing to do with my
presenting problem.

The downside is that when I add or delete assertions, I have to do the
tedious job of renumbering, so that later maintainers can
follow the intent. Paying for my transgressions.

It takes a developer minutes of time to place a message, but it can save a debugger hours.

Lovely piece of advice :) Using CollectorsTestOnJDK12 as a template I tried to add some similar clues to the assertions, let me know if that's what you're looking for here.

javalib/src/main/scala/java/net/HttpCookie.scala

LeeTibbert · 2024-05-23T04:27:54Z

unit-tests/shared/src/test/scala/org/scalanative/testsuite/javalib/net/HttpCookieTest.scala

+    assert(!HttpCookie.domainMatches("website.com", "awebsite.com"))
+  }
+
+}


One can spend eternities one does not have writing tests
for failure cases. Users are so good at generating just
the failure case not detected by tests. Such is life.

The code goes to some length to detectIllegalArgumentException("Illegal cookie name").

Did I miss a Test for that? If not, you might want to consider adding it.

(is the "Illegal" in the message from JVM? If not, could it be changed
to something which does not keep me up all day wondering if the Sheriff's Deputy is coming for me because I typo-ed a name)

is the "Illegal" in the message from JVM?

Yes, I tried to mimic the behavior of the JVM; at least to a reasonable degree. Added tests for that now as well

LeeTibbert · 2024-05-23T04:38:40Z

javalib/src/main/scala/java/net/HttpCookie.scala

+  }
+
+}
+


There may need to be a javadoc/scaladoc comment here
so that the documentation build picks this up as a supported class.

@ekrich What is the current process for a newly supported class
to get picked up by the documentation?

I blinked and the process changed. Thanks.

LeeTibbert · 2024-05-23T04:42:49Z

domaspoliakas

Thank you for this contribution. I hope it is the first of many.

Overall, it looks very good. It is nice to see the corresponding & careful Tests.

I did my traditional "What documentation impacts does this PR have?" and
found out I/we need a short tutorial on current practice from @ekrich.
I think the required change is to add a javadoc/scaladoc (i.e. ``/** HttpCookie*/`) comment before
the new class. I may be way off base here. Guess this PR gives me the opportunity to
learn something contemporary.

Enough for tonight, I will check the log files by the light of day to see what is failing & try to
puzzle out why.

LeeTibbert · 2024-05-23T04:45:43Z

To allow my mind to rest and focus on my current project, I did a spot test of perhaps 3 failing
tests. They were all the same:

    while (i < unprocessed.size) {
[error]    |                 ^^^^^^^^^^^^^^^^
[error]    |         method size in trait Collection must be called with () argument

Hope that helps.

LeeTibbert · 2024-05-23T13:45:01Z

As promised, FYI:
https://alvinalexander.com/scala/how-to-define-equals-hashcode-methods-in-scala-object-equality/

I found this article helpful. In it Alvin references at least two of his original sources. Alvin is careful
and informed, not "just some joe off the Internet".

An almost identical descriptions occur on scala-lang.org: my guess is that the two are related.
Probably both his work.

If I understand HttpCookie correctly, it can be extended which means that it can not be marked final.
That means that one must figure out a proper canEquals (case? I am writing from memory).

Building up enough recent models of the current recommended practice will make it easier for people
to find the models.

LeeTibbert · 2024-05-23T13:48:16Z

Any idea what the chance of a hash collision is from "concatenationOfAllFields".##? I sure do not
know and have no idea how to figure that math. Likely to be relatively expensive at runtime but
good, easy devo bootstrapping.

domaspoliakas · 2024-05-24T10:13:13Z

Addressed some of your comments wrt equals, hashcode and tests.

For hashcode I used name, value, domain and path as that seems to be what the JVM is doing as well.

Also it turns out the JVM keeps track of the time when the object was created for the purpose of evaluating hasExpired(), which seems crazy to me but nevertheless I have also replicated here now.

WojciechMazur

Thank you for the contribution! It seams to not be passing the CI - it's failing due to incompatibilities with the JVM semantics - we ran javalib tests on both JVM (first) and then using Native. Because it fails on the JVM it means that tests are incorrect and don't follow JVM semantics. You can run these locally using testsJVM3/test

javalib/src/main/scala/java/net/HttpCookie.scala

WojciechMazur · 2024-05-24T12:02:13Z

javalib/src/main/scala/java/net/HttpCookie.scala

+  def getValue(): String = this._value
+  def getVersion(): Int = this._version
+  def hasExpired(): Boolean = {
+    _maxAge >= 0 && ((System


JVM would return true if maxAge == 0, so we should start with 2 quick range checks before starting getting current time

I'm not sure I quite follow what you mean wrt range checks, let me know if the new version is ok

javalib/src/main/scala/java/net/HttpCookie.scala

WojciechMazur · 2024-05-24T12:21:26Z

javalib/src/main/scala/java/net/HttpCookie.scala

+        if (header.regionMatches(false, 0, "set-cookie2:", 0, 12)) {
+          header.substring(12)
+        } else if (header.regionMatches(false, 0, "set-cookie:", 0, 11))
+          header.substring(11)


You're always matching the start of the string you can simply use header.startsWith("...") to make it more readable.

startsWith is case-sensitive, which is why I use regionMatches here

javalib/src/main/scala/java/net/HttpCookie.scala

domaspoliakas · 2024-05-24T18:51:05Z

Addressed the PR comments now. There's one mismatch between the JVM behavior and my implementation that I'm still trying to track down though, I'll keep working on it in the meantime.

domaspoliakas · 2024-05-24T19:10:20Z

unit-tests/shared/src/test/scala/org/scalanative/testsuite/javalib/net/HttpCookieTest.scala

+  @Test
+  def stringEscapingTest(): Unit = {
+    // I named this test "string escaping", but really it's more about how
+    // the jvm does no such thing
+    val out =
+      HttpCookie.parse(
+        "set-cookie2:potato=\"toma\\\"t,o=asdf\",second=second;comment=\"something;discard;portlist=123\""
+      )
+    assertEquals("number of cookies", 2, out.size())
+    val cookieOne = out.get(0)
+    assertCookie(
+      cookieOne,
+      "cookie one",
+      name = "potato",
+      value = "\"toma\\\"t"
+    )
+    // despite the fact that the first comma is in a quoted string - the JVM
+    // still splits it into a separate cookie at that point. Hence we must do so as well
+    //
+    // Similarly for semicolons - the JVM doesn't seem to distinguish between semicolons
+    // in strings and those outside, and hence we do the same
+    val cookieTwo = out.get(1)
+    assertCookie(
+      cookieTwo,
+      "cookie two",
+      name = "o",
+      value = "asdf\",second=second",
+      comment = "\"something",
+      discard = true
+    )


The way the JVM splits on the comma inside a quoted string in one place, but then totally ignores another comma entirely is just baffling. I'll sleep on this and hopefully get it sorted over the weekend.

javalib/src/main/scala/java/net/HttpCookie.scala

WojciechMazur · 2024-05-25T10:58:02Z

javalib/src/main/scala/java/net/HttpCookie.scala

+        nameless.split(",")
+        // splitUnquotedAt(nameless, ',')
+
+      Arrays.asList(unprocessed).forEach { kv =>


After changing splitUnquotedAt to split you're having a Array[String] as a result, you can use foreach directly on it without wrapping it into Java List

Suggested change

Arrays.asList(unprocessed).forEach { kv =>

unprocessed.foreach { kv =>

HttpCookie and its tests

7f18d58

domaspoliakas mentioned this pull request May 21, 2024

Scala-Native Support (1000USD Bounty) com-lihaoyi/requests-scala#156

Open

LeeTibbert reviewed May 23, 2024

View reviewed changes

javalib/src/main/scala/java/net/HttpCookie.scala Outdated Show resolved Hide resolved

LeeTibbert reviewed May 23, 2024

View reviewed changes

HttpCookie: more tests, more docs, some bugfixes

3433d78

Formatting

c37e1e1

WojciechMazur requested changes May 24, 2024

View reviewed changes

domaspoliakas added 2 commits May 24, 2024 18:38

HttpCookie - various refactors to address PR comments

d4400d8

HttpCookie WIP to match JVM semantics

ab88ad5

domaspoliakas commented May 24, 2024

View reviewed changes

WojciechMazur reviewed May 25, 2024

View reviewed changes

lihaoyi mentioned this pull request May 27, 2024

Scala-Native Support (500USD Bounty) lihaoyi/test#656

Open

	Arrays.asList(unprocessed).forEach { kv =>
	unprocessed.foreach { kv =>

HttpCookie and its tests #3927

Are you sure you want to change the base?

HttpCookie and its tests #3927

Uh oh!

Conversation

domaspoliakas commented May 21, 2024

Uh oh!

domaspoliakas commented May 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeeTibbert commented May 23, 2024

Uh oh!

LeeTibbert commented May 23, 2024

Uh oh!

LeeTibbert commented May 23, 2024

Uh oh!

LeeTibbert commented May 23, 2024

Uh oh!

domaspoliakas commented May 24, 2024

Uh oh!

WojciechMazur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

domaspoliakas commented May 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WojciechMazur left a comment •

edited

Loading