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

Remove reflections #117

Merged
merged 4 commits into from
Sep 16, 2023
Merged

Conversation

hoangmaihuy
Copy link
Contributor

Changes being made

Remove reflections from dependencies. I've made changes in two places:

  • ClassTagUtils: Simply replace reflections usage with Java's reflection methods
  • ScalapbPayloadConverter: reflections is being used for scanning Scalapb's companion objects, but instead we can get companion object from valueClass.

Additional context

reflections is not under active development and maintenance, and it has serveral issues on newer JDKs, please see ronmamo/reflections#440 and ronmamo/reflections#377 for example.

@vitaliihonta
Copy link
Owner

Hi @hoangmaihuy! Thank you so much for your contribution! That's a very clever change. I didn't expect it's that straightforward to drop reflections 😄
Before merging, I will check if everything works fine in my example projects. I hope to do it soon

@vitaliihonta
Copy link
Owner

I found some issues while decoding ADTs. I will figure it out on my own 😃

@hoangmaihuy
Copy link
Contributor Author

@vitaliihonta Thank you a lot for this great work, glad I can help! 😄

@vitaliihonta
Copy link
Owner

TODOs

  • Check if the default ScalaPB generated oneOf's work work
  • Develop a workaround for classes generated using sealedValue

@hoangmaihuy
Copy link
Contributor Author

@vitaliihonta Workaround for sealedValue might be like this, I've tested on my local and it worked!
Please let me know if it works on your example projects

  private def getClassCompanion[T](
    valueClass: Class[T]
  ): GeneratedMessageCompanion[GeneratedMessage] = {
    val companionClass = Class.forName(valueClass.getName + "$")
    companionClass
      .getDeclaredField(scalaModuleField)
      .get(null)
      .asInstanceOf[GeneratedMessageCompanion[GeneratedMessage]]
  }

  private def getCompanion[T](
    typeUrl: String,
    valueClass: Class[T]
  ): GeneratedMessageCompanion[GeneratedMessage] = {
    println(s"typeUrl = $typeUrl")
    println(s"valueClass = ${valueClass.getName}")
    if (valueClass.isInterface && valueClass.getInterfaces.contains(classOf[GeneratedSealedOneof])) {
      val subClassType = typeUrl.split("\\.").last
      val subClassName = valueClass.getName.replace(valueClass.getSimpleName, subClassType)
      println(s"subClassName = ${subClassName}")
      getClassCompanion(Class.forName(subClassName))
    } else {
      getClassCompanion(valueClass)
    }
  }

# Conflicts:
#	integration-tests/src/test/scala/zio/temporal/protobuf/internal/ProtoFileObjectAutoLoaderSpec.scala
@vitaliihonta
Copy link
Owner

vitaliihonta commented Sep 16, 2023

Hey @hoangmaihuy, could you merge my updates from #120 into your branch? I cannot push my changes to your fork directly, but I want to keep you in the commit history 😃

Your solution works perfectly! I just had to adapt it for some internal corner cases

@hoangmaihuy
Copy link
Contributor Author

@vitaliihonta I've merged #120 into this branch 😄

@codecov-commenter
Copy link

Codecov Report

Merging #117 (1b954c3) into main (0b8b0dd) will decrease coverage by 0.63%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
- Coverage   39.05%   38.43%   -0.63%     
==========================================
  Files         116      115       -1     
  Lines        4127     4046      -81     
==========================================
- Hits         1612     1555      -57     
+ Misses       2515     2491      -24     
Files Changed Coverage Δ
.../zio/temporal/protobuf/ProtobufDataConverter.scala 0.00% <0.00%> (ø)
...in/scala/zio/temporal/internal/ClassTagUtils.scala 77.14% <83.33%> (+8.09%) ⬆️
...io/temporal/protobuf/ScalapbPayloadConverter.scala 90.83% <92.50%> (-2.56%) ⬇️

... and 6 files with indirect coverage changes

@vitaliihonta vitaliihonta merged commit 82b4c76 into vitaliihonta:main Sep 16, 2023
14 checks passed
@vitaliihonta
Copy link
Owner

@hoangmaihuy, thank you so much for your contribution 🙌
I'm going to release it in 0.5.0 as makeAutoload method was removed

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.

None yet

3 participants