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

Change Uri.Path to be own type #3221

Merged
merged 1 commit into from May 25, 2020
Merged

Conversation

hamnis
Copy link
Contributor

@hamnis hamnis commented Feb 25, 2020

This changes Uri.Path to be an actual class to encapsulate Path segments.
We have two boolean flags here. These can be removed by transforming
the Path type to an own ADT hierarchy.

The main motivation for this change is to represent paths as something more than a String.

I have used scala.collection.immutable.Vector instead of List as Vector allows O(1)
appends.

This change is binary incompatible.

Typeclasses:

  • Eq
  • Semigroup
  • Adding a monoid is possible, but there are two zero cases. Changing the Path type to an ADT will resolve that.

We have a few convenince functions here:

  • startWith to be able to use in the Routers
  • indexOf and splitAt to allow us to replace the caret stuff.
  • normalize: drops empty segments
  • toAbsolute and toRelative
  • merge: using RFC3986 rules
  • concat

Unrelated, add out to .gitignore to be able to use bloop from intellij

Representing Encoded Strings as just Strings are not really useful, so we have added a Segment class to have encoded values in.

  • Use uri and path string interpolation in tests

Request changes:

  • scriptName and pathInfo changed to be Uri.Path

Copy link
Member

@rossabaker rossabaker left a comment

Thanks. I think this is headed in the right direction. I have more questions than answers.

core/src/main/scala/org/http4s/Uri.scala Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
final class Path(
val segments: Chain[String],
val absolute: Boolean = false,
val endsWithSlash: Boolean = false)
Copy link
Member

@rossabaker rossabaker Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me you could get away with an empty segment at the beginning to represent absolute, and an empty segment at the end to represent endsWithSlash, and not need three variables. But maybe that complicates other logic.

Copy link
Member

@rossabaker rossabaker Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your idea of the ADT would move toward being able to represent absolute URIs at the type level, and could help solve the problem in #3210. But it would need to carry forward to Uri. Probably complicates things too much. But ... maybe?

core/src/main/scala/org/http4s/syntax/StringSyntax.scala Outdated Show resolved Hide resolved
@hamnis hamnis force-pushed the uri-path-segments branch 4 times, most recently from 2812792 to 43435f3 Compare Mar 12, 2020
@hamnis hamnis added the breaking Change breaks compatibility (binary or source) label Apr 28, 2020
Copy link
Member

@rossabaker rossabaker left a comment

As we discussed on Gitter, storing encoded Strings is definitely not the right model. We should internally store a newtype of encoded strings, or store decoded strings and encode them on the fly. I'm leaning toward the latter option, but I think both are defensible.

core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
Copy link
Member

@rossabaker rossabaker left a comment

Looking good. Noticed a couple more nits, but I think we're about ready finally.

core/src/main/scala/org/http4s/Uri.scala Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Show resolved Hide resolved
core/src/main/scala/org/http4s/Uri.scala Outdated Show resolved Hide resolved
)
}

implicit val eq: Eq[Path] = Eq.fromUniversalEquals[Path]
Copy link
Member

@rossabaker rossabaker May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot more are possible here. Hash, Order. I think Monoid. LowerBounded, if you want to get exotic. But it's more important to get this landed, and we can circle back to those.

Copy link
Contributor Author

@hamnis hamnis May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this semigroup passes all the laws. I guess a test is in order

Copy link
Member

@rossabaker rossabaker May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've even got a Monoid with empty.

We should decide if we want this to be an Order or not. It could be. I chose not to provide an Order[Method] in my recent reforms there, because I didn't know that alphabetical order was compelling. It's perhaps a bit more useful to provide a stable ordering of URIs, of which this would be a part.

Copy link
Contributor Author

@hamnis hamnis May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two monoids, both Uri.Path.empty and Uri.Path.Root
Although you might simulate the Root one with adding a toAbsolute after and vise versa with toRelative

Copy link
Member

@rossabaker rossabaker May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Hmm. I was thinking of empty as being the empty. Maybe we can sidestep the ambiguity by just providing Semigroup? I would need to think through the cases a bit more deeply.

core/src/main/scala/org/http4s/Uri.scala Show resolved Hide resolved
core/src/main/scala/org/http4s/Message.scala Outdated Show resolved Hide resolved

/**
* Adds the path exactly as described. Any path element must be urlencoded ahead of time.
* @param path the path string to replace
*/
def withPath(path: Path): Uri = copy(path = path)
@deprecated("Use {withPath(Uri.Path)} instead", "1.0.0-M1")
Copy link
Contributor Author

@hamnis hamnis May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since should be checked by someone

Copy link
Member

@rossabaker rossabaker May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that {} syntax do anything in a deprecation warning? I sometimes backtick them. Doesn't really matter. Mostly asking for my own curiosity.

Copy link
Contributor Author

@hamnis hamnis May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I`m not sure. For some reason i was thinking scaladoc syntax here. I can change it to backticks if that is what we have used elsewhere

)
}

implicit val eq: Eq[Path] = Eq.fromUniversalEquals[Path]
Copy link
Contributor Author

@hamnis hamnis May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this semigroup passes all the laws. I guess a test is in order

Copy link
Member

@rossabaker rossabaker left a comment

I think it's ready as soon as we have a test for the instances.


/**
* Adds the path exactly as described. Any path element must be urlencoded ahead of time.
* @param path the path string to replace
*/
def withPath(path: Path): Uri = copy(path = path)
@deprecated("Use {withPath(Uri.Path)} instead", "1.0.0-M1")
Copy link
Member

@rossabaker rossabaker May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that {} syntax do anything in a deprecation warning? I sometimes backtick them. Doesn't really matter. Mostly asking for my own curiosity.

core/src/main/scala/org/http4s/Uri.scala Show resolved Hide resolved
)
}

implicit val eq: Eq[Path] = Eq.fromUniversalEquals[Path]
Copy link
Member

@rossabaker rossabaker May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've even got a Monoid with empty.

We should decide if we want this to be an Order or not. It could be. I chose not to provide an Order[Method] in my recent reforms there, because I didn't know that alphabetical order was compelling. It's perhaps a bit more useful to provide a stable ordering of URIs, of which this would be a part.

@hamnis
Copy link
Contributor Author

@hamnis hamnis commented May 18, 2020

How should we handle 991d59b ? scalafix or equals hack?

@hamnis hamnis marked this pull request as ready for review May 18, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 18, 2020

Is the issue in 991d59b that code is calling path.equals(string), and it worked when it was a type alias and was string.equals(string)? So it'll be a silent failure? Damned universal equality.

I think an asymmetric equals, which violates both the laws of Scala and Java, would be more problematic in the long run. We could perhaps log a warning in the equals method, but that's still a runtime solution. I think a scalafix lint rule (or fix) is the best way forward.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 18, 2020

If it weren't Uri.Path, but just Path, would the situation be better? We wouldn't be changing the equality of an existing type, but people are just going to change their imports and not think about the ramifications. That probably doesn't save us?

@hamnis
Copy link
Contributor Author

@hamnis hamnis commented May 18, 2020

that would not help us :( Universal equality are biting us here, and it still would with a new type.
Logging on String would make it visible at least, even though runtime...

I agree that we want a scalafix, and I also think that it should be a different PR for that.

Copy link
Member

@rossabaker rossabaker left a comment

I'm happy with this. We can decide on a Monoid.empty and add a scalafix separately.

@hamnis
Copy link
Contributor Author

@hamnis hamnis commented May 19, 2020

I would like a second approval on this.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 19, 2020

Sorry, I just gave you a merge conflict. And will probably be giving you another, as I remove that internal implementation of hashLower from 1.0.

This changes Uri.Path to be an actual class to encapsulate Path segments.
We have two boolean flags here. These can be removed by transforming
the `Path` type to an own `ADT` hierarchy.

The main motivation for this change is to represent paths as something more than a String.

I have used `scala.collection.immutable.Vector` instead of `List` as `Vector` allows O(1)
appends.

This change is binary incompatible.

Typeclasses:
* Eq
* Semigroup
* Adding a monoid is possible, but there are two zero cases. Changing the Path type to an ADT will resolve that.

We have a few convenince functions here:

* startWith to be able to use in the Routers
* indexOf and splitAt to allow us to replace the caret stuff.
* normalize: drops empty segments
* toAbsolute and toRelative
* merge: using RFC3986 rules
* concat

Unrelated, add `out` to .gitignore to be able to use bloop from intellij

Representing Encoded Strings as just Strings are not really useful, so we have added a Segment class to have encoded values in.

* Use uri and path string interpolation in tests

Request changes:
* scriptName and pathInfo changed to be Uri.Path
@hamnis hamnis merged commit 52c07bb into http4s:master May 25, 2020
10 checks passed
@hamnis hamnis deleted the uri-path-segments branch May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change breaks compatibility (binary or source)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants