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

fix breaking change in MediaType.toString #334

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

fwbrasil
Copy link
Contributor

@fwbrasil fwbrasil commented Mar 4, 2024

Problem

#331 changed MediaType.toString to become a lazy val, which is a breaking change. I'm seeing this issue while trying to publish a version of Tapir with the optimizations mentioned in softwaremill/tapir#3552.

Solution

Make the lazy val private and keep the same toString() interface as before.

@fwbrasil fwbrasil force-pushed the fix-mediatype-string branch from a0fa7f0 to 80fa8cf Compare March 4, 2024 18:36
@adamw
Copy link
Member

adamw commented Mar 4, 2024

Oh that's interesting ... what kind of errors do you see? Mima seems happy, but apparently there's more to compatibility ;)

@@ -63,7 +63,7 @@ case class MediaType(

def isModel: Boolean = mainType.equalsIgnoreCase("model")

override lazy val toString: String = {
private lazy val toStringCache: String = {
Copy link
Member

Choose a reason for hiding this comment

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

maybe you could add a comment explaining why this particular class has a toStringCache field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@fwbrasil
Copy link
Contributor Author

fwbrasil commented Mar 4, 2024

Maybe it's binary-compatible because Scala will still generate the overriden toString but it's not source-compatible because it's not possible to call the method with empty parenthesis. The error:

[error] /Users/fwbrasil/workspace/sttp-model/core/src/test/scala/sttp/model/MediaTypeTests.scala:65:16: not enough arguments for method apply: (index: Int)Char in class StringOps.
[error] Unspecified value parameter index.
[error]     mt.toString() shouldBe "application/gzip"
[error]                ^

@fwbrasil fwbrasil force-pushed the fix-mediatype-string branch from 80fa8cf to 9db38a3 Compare March 4, 2024 20:08
@adamw
Copy link
Member

adamw commented Mar 4, 2024

Ah, makes sense, thanks :) @kciesielski didn't we add a similar optimization elsewhere recently? Maybe we need to add a similar work-around

@adamw adamw merged commit 4df0e52 into softwaremill:master Mar 4, 2024
4 checks passed
@adamw
Copy link
Member

adamw commented Mar 4, 2024

thx :)

@kciesielski
Copy link
Member

@adamw I did this to Uri but didn't publish a PR, just mentioned to you that it would break one toString() call in Tapir and that I didn't see this source incompatibility as a big issue.

@adamw
Copy link
Member

adamw commented Mar 5, 2024

@kciesielski ah ... well it would be good to maintain source compat as well, so we'll need the same logic there as well, if you introduce the change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants