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

mutating protobuf.impl.flatland.core.FlatlandProtoBuf #41

Open
barkanido opened this issue Apr 4, 2019 · 2 comments
Open

mutating protobuf.impl.flatland.core.FlatlandProtoBuf #41

barkanido opened this issue Apr 4, 2019 · 2 comments

Comments

@barkanido
Copy link

barkanido commented Apr 4, 2019

Hey folks, I am evaluating this and testing some simple scenarios of mutation. What I am trying to test is:

  1. creating a proto object
  2. mutate in some Clojure map-related API (that is allowed by the schema)
  3. serialize to bytes
  4. deserialize and see that the mutation actually worked:

now, given this schema:

syntax = "proto2";

package clojure_protobuf_poc.core.person;

option java_outer_classname = "PersonProto";

message Name {
  optional string name = 1;
  optional string surname = 2;
}

message Person {
  optional int32  id    = 1;
  optional Name name  = 2;
  optional string email = 3;
  repeated string likes = 4;
}

and this test code:

(ns clojure-protobuf-poc.core-test
  (:require [clojure.test :refer [testing deftest is]]
            [protobuf.core :as protobuf])
  (:import [clojure_protobuf_poc.core.person PersonProto$Name PersonProto$Person]
           [protobuf.impl.flatland.core FlatlandProtoBuf]))

(defn bytes->Person [person-bytes]
  (protobuf/bytes-> (protobuf/create PersonProto$Person) person-bytes))

(defn mutate->round-trip [pb f]
  (let [mutated (f pb)]
    (is (= FlatlandProtoBuf (type mutated)))
    (-> mutated 
        protobuf/->bytes 
        bytes->Person)))

(deftest mutation
  (let [alice-name (protobuf/create PersonProto$Name {:name "Alice" :surname "Cohen"})
        alice (protobuf/create PersonProto$Person {:id 108 :name alice-name :email "[email protected]"})]
    (println alice)
    (testing "no mutation"
      (let [alice-tag (mutate->round-trip alice identity)]
        (is (= alice-tag alice))))
    (testing "dissoc"
      (let [no-id (mutate->round-trip alice #(dissoc % :id))]
        (is (= nil (:id no-id)))))
    (testing "assoc-in"
      (let [new-surname (mutate->round-trip alice #(assoc-in % [:name :surname] "Levi"))]
        (is (= "Levi" (get-in new-surname [:name :surname])))))
    (testing "update-in"
      (let [no-surname (mutate->round-trip alice #(update-in % [:name] dissoc :surname))]
        (is (= nil (get-in no-surname [:name :surname]))))) ; fails-> we get "Cohen"
    (testing "assoc"
      (let [new-email1 (mutate->round-trip alice #(assoc % :email "new-email"))]
        (is (= "new-email" (:email new-email1)))))
    (testing "merge" 
      ; crashes cause after the merge we get  java.lang.IllegalArgumentException: No implementation of 
      ; method: :->bytes of protocol: #'protobuf.core/ProtoBufAPI 
      ; found for class: protobuf.PersistentProtocolBufferMap
      (let [new-email2 (mutate->round-trip alice #(merge % {:email "new-email"}))]
        (is (= "new-email" (:email new-email2)))))))

tests fail with:

FAIL in (mutation) (core_test.clj:31)
update-in
expected: nil
  actual: "Cohen"
    diff: + "Cohen"

lein test :only clojure-protobuf-poc.core-test/mutation

FAIL in (mutation) (core_test.clj:12)
merge
expected: protobuf.impl.flatland.core.FlatlandProtoBuf
  actual: protobuf.PersistentProtocolBufferMap
    diff: - protobuf.impl.flatland.core.FlatlandProtoBuf
          + protobuf.PersistentProtocolBufferMap

lein test :only clojure-protobuf-poc.core-test/mutation

ERROR in (mutation) (core_deftype.clj:583)
Uncaught exception, not in assertion.
expected: nil
  actual: java.lang.IllegalArgumentException: No implementation of method: :->bytes of protocol: #'protobuf.core/ProtoBufAPI found for class: protobuf.PersistentProtocolBufferMap
 at clojure.core$_cache_protocol_fn.invokeStatic (core_deftype.clj:583)
...
...

Can you explain why update-in and merge aren't supported and what I (probably) have done wrong?
Thanks!

Maybe, put differently, it looks like PersistentProtocolBufferMap isn't 100% compatible with a Clojure map:

  1. Is that correct?
  2. If so, should I strive to convert it into a Clojure map? What is the fastest way to do so (recursively)?
@fr33m0nk
Copy link

fr33m0nk commented Aug 18, 2020

@barkanido I had faced similar issue when the protobuf message was made of nested proto messages and if the nested message was being enriched with data and assoc-in was not yielding desired result.

To overcome this problem, I came up with following Clojure protocol which converts protobuf object to map:

(defn as-map [f o]
  (reduce (fn [m [k v]]
            (assoc m k (f v)))
          {} o))

(defprotocol ProtoBufToClojureMap
  (->clj-map [o]))

(extend-protocol ProtoBufToClojureMap
  protobuf.impl.flatland.core.FlatlandProtoBuf
  (->clj-map [o]
    (as-map ->clj-map o))

  protobuf.PersistentProtocolBufferMap
  (->clj-map [o]
    (as-map ->clj-map o))

  java.lang.Object
  (->clj-map [o] o))

Above protocol converts the protobuf map into a Clojure map, allowing you to do all sort of map operations

Usage is pretty straight forward as well:

(def alice-as-clj-map (->clj-map alice))

You can covert it back to protobuf object for publishing by using the standard library functions.

@oubiwann Please let me know if this seems fit enough for PR. This may sit in some utility name-space.

@fr33m0nk
Copy link

fr33m0nk commented May 7, 2022

@oubiwann I have create a PR to resolve this Issue.
This PR resolvesthe issue and :

  • Adds additional Persistent Map behaviours: assoc-in, update-in, merge and merge-with to protobuf.impl.flatland.core.FlatlandProtoBuf and protobuf.PersistentProtocolBufferMap allowing users to work with constructed objects as if they were regular Clojure maps.
  • Updates version to 3.6.1-v1.0-SNAPSHOT

Please review it in your time 😄
PS: As a well maintained Protobuf opertaional alternative, I would recommend using AppsFlyer/pronto though.

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 a pull request may close this issue.

2 participants