scalapb / ScalaPB

Protocol buffer compiler for Scala.
https://scalapb.github.io/
Apache License 2.0
1.3k stars 278 forks source link

FieldMaskUtil equivalent #373

Closed talpr closed 5 years ago

talpr commented 6 years ago

Standard gRPC API design guidelines suggest using google.protobuf.FieldMask for "filtering" message fields (useful for partial updates or reading only parts of a message). The Java PB implementation provides a FieldMaskUtil class that lets one combine masks and "apply" them to messages.

When working with ScalaPB, there is no equivalent for this, and the easiest way I found to use field masks seems to be:

  1. Convert the message and the mask to Java protos
  2. Do your processing (add fields to the mask, intersect with other masks, merge/"apply" masks to messages)
  3. Convert the message back to Scala

So the question is, should ScalaPB have some similar Scala API? (If so, I'm more than happy to try implementing it and opening a PR)

thesamet commented 6 years ago

Yes, I think ScalaPB should provide support for this. PR will be welcomed. When working on this keep in mind that we generally want to follow the semantics and naming conventions of the Java API to avoid surprising the user. For v1 you can consider delegating some of the work to Google's Java implementation by using Java conversion. This wouldn't work for things like isValid and merge so you'll have to write it using ScalaPB's PMessage. The json related methods should go to scalapb-json4s. Finally, the test suite for the Java implemention needs to be ported as well to prove that it's compatible.

Hope this helps, don't hesitate to send any questions you may have.

-nadav

On Dec 1, 2017 2:11 AM, "Tal Pressman" notifications@github.com wrote:

Standard gRPC API design guidelines suggest using google.protobuf.FieldMask for "filtering" message fields (useful for partial updates or reading only parts of a message). The Java PB implementation provides a FieldMaskUtil class that lets one combine masks and "apply" them to messages.

When working with ScalaPB, there is no equivalent for this, and the easiest way I found to use field masks seems to be:

  1. Convert the message and the mask to Java protos
  2. Do your processing (add fields to the mask, intersect with other masks, merge/"apply" masks to messages)
  3. Convert the message back to Scala

So the question is, should ScalaPB have some similar Scala API? (If so, I'm more than happy to try implementing it and opening a PR)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/scalapb/ScalaPB/issues/373, or mute the thread https://github.com/notifications/unsubscribe-auth/AASwrV6ee-PlfOmCyNc0XV79zD1O9-Xwks5s78LMgaJpZM4QyD4s .

talpr commented 6 years ago

Cool, I'll find some time this week and start working on it.

perehonchuk commented 6 years ago

@thesamet, is following api will be ok?

1) FieldMaskUtil.merge will be in your message, e.g:

FieldMaskUtil.merge(fieldMask, src, dest) --> src.applyMask(fieldMask) // returns new message

We want to do it like an implicit class, so if you wanna use it you should import it before:

import com.youcompany.api.YourMessageOps._

2) All another methods will be placed in some object like FieldMaskUtil but scala. Current isValid implementations with class

isValid(Class<? extends Message> type, ...) 

uses reflection and will be replaced with

isValid[A <: GeneratedMessage with Message[A]](type: scalapb.GeneratedMessageCompanion[A]): Boolean

so user will be able to call it like:

isValid(WorldRequest, ....)
thesamet commented 6 years ago

@perehonchuk the API looks good. So applyMask would be provided through some new implicit class named scalapb.FieldMaskOps? I didn't understand why it goes under com.yourcompany. - was the intention that we generate it for each class? (Ideally, we can have one generic implementation)

thesamet commented 5 years ago

Closing this old issue since looks like there is no demand for this functionality. Happy to re-open if anyone would like to work on this.