Register asset with keyword metadata fails

shark8me commented 5 years ago

Registering an asset with keyword metadata throws a JSON parse exception

 (testing "registration with string metadata "
    (let [a1 (s/asset (s/memory-asset {:meta :data} "test asset"))
          remote-asset (s/register (get-remote-agent)a1)]
      (is (s/asset? remote-asset))))

lein test :only integration.test-surfer/register-with-surfer

ERROR in (register-with-surfer) (JSONObjectCache.java:38)
Uncaught exception, not in assertion.
expected: nil
  actual: java.lang.Error: Error in JSON parsing: null
 at sg.dex.starfish.util.JSONObjectCache.parse (JSONObjectCache.java:38)
    sg.dex.starfish.impl.AAsset.getMetadata (AAsset.java:49)
    sg.dex.starfish.impl.remote.RemoteAgent.registerAsset (RemoteAgent.java:219)
    starfish.core$register.invokeStatic (core.cljc:373)
    starfish.core$register.invoke (core.cljc:370)
    integration.test_surfer$fn__808$fn__817.invoke (test_surfer.clj:28)
    integration.test_surfer$fn__808.invokeStatic (test_surfer.clj:25)
    integration.test_surfer/fn (test_surfer.clj:18)
    clojure.test$test_var$fn__9737.invoke (test.clj:717)
    clojure.test$test_var.invokeStatic (test.clj:717)
    clojure.test$test_var.invoke (test.clj:708)
    clojure.test$test_vars$fn__9763$fn__9768.invoke (test.clj:735)
    clojure.test$default_fixture.invokeStatic (test.clj:687)
    clojure.test$default_fixture.invoke (test.clj:683)
    clojure.test$test_vars$fn__9763.invoke (test.clj:735)
    clojure.test$default_fixture.invokeStatic (test.clj:687)
    clojure.test$default_fixture.invoke (test.clj:683)
    clojure.test$test_vars.invokeStatic (test.clj:731)
    clojure.test$test_all_vars.invokeStatic (test.clj:737)
    clojure.test$test_ns.invokeStatic (test.clj:758)
    clojure.test$test_ns.invoke (test.clj:743)
    user$eval224$fn__345.invoke (form-init6789831084988863437.clj:1)
    clojure.lang.AFn.applyToHelper (AFn.java:156)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.core$apply.invokeStatic (core.clj:667)
    clojure.core$apply.invoke (core.clj:660)
    leiningen.core.injected$compose_hooks$fn__154.doInvoke (form-init6789831084988863437.clj:1)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.core$apply.invokeStatic (core.clj:665)
    clojure.core$apply.invoke (core.clj:660)
    leiningen.core.injected$run_hooks.invokeStatic (form-init6789831084988863437.clj:1)
    leiningen.core.injected$run_hooks.invoke (form-init6789831084988863437.clj:1)
    leiningen.core.injected$prepare_for_hooks$fn__159$fn__160.doInvoke (form-init6789831084988863437.clj:1)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.lang.AFunction$1.doInvoke (AFunction.java:31)
    clojure.lang.RestFn.invoke (RestFn.java:408)
    clojure.core$map$fn__5866.invoke (core.clj:2755)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:51)
    clojure.lang.Cons.next (Cons.java:39)
    clojure.lang.RT.boundedLength (RT.java:1792)
    clojure.lang.RestFn.applyTo (RestFn.java:130)
    clojure.core$apply.invokeStatic (core.clj:667)
    clojure.test$run_tests.invokeStatic (test.clj:768)
    clojure.test$run_tests.doInvoke (test.clj:768)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.core$apply.invokeStatic (core.clj:665)
    clojure.core$apply.invoke (core.clj:660)
    user$eval224$fn__357$fn__410.invoke (form-init6789831084988863437.clj:1)
    user$eval224$fn__357$fn__358.invoke (form-init6789831084988863437.clj:1)
    user$eval224$fn__357.invoke (form-init6789831084988863437.clj:1)
    user$eval224.invokeStatic (form-init6789831084988863437.clj:1)
    user$eval224.invoke (form-init6789831084988863437.clj:1)
    clojure.lang.Compiler.eval (Compiler.java:7177)
    clojure.lang.Compiler.eval (Compiler.java:7167)
    clojure.lang.Compiler.load (Compiler.java:7636)
    clojure.lang.Compiler.loadFile (Compiler.java:7574)
    clojure.main$load_script.invokeStatic (main.clj:475)
    clojure.main$init_opt.invokeStatic (main.clj:477)
    clojure.main$init_opt.invoke (main.clj:477)
    clojure.main$initialize.invokeStatic (main.clj:508)
    clojure.main$null_opt.invokeStatic (main.clj:542)
    clojure.main$null_opt.invoke (main.clj:539)
    clojure.main$main.invokeStatic (main.clj:664)
    clojure.main$main.doInvoke (main.clj:616)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.lang.Var.applyTo (Var.java:705)
    clojure.main.main (main.java:40)
Caused by: org.json.simple.parser.ParseException: null
 at org.json.simple.parser.Yylex.yylex (Yylex.java:610)
    org.json.simple.parser.JSONParser.nextToken (JSONParser.java:269)
    org.json.simple.parser.JSONParser.parse (JSONParser.java:118)
    org.json.simple.parser.JSONParser.parse (JSONParser.java:81)
    org.json.simple.parser.JSONParser.parse (JSONParser.java:75)
    sg.dex.starfish.util.JSONObjectCache.parse (JSONObjectCache.java:34)
    sg.dex.starfish.impl.AAsset.getMetadata (AAsset.java:49)
    sg.dex.starfish.impl.remote.RemoteAgent.registerAsset (RemoteAgent.java:219)
    starfish.core$register.invokeStatic (core.cljc:373)
    starfish.core$register.invoke (core.cljc:370)
    integration.test_surfer$fn__808$fn__817.invoke (test_surfer.clj:28)
    integration.test_surfer$fn__808.invokeStatic (test_surfer.clj:25)
    integration.test_surfer/fn (test_surfer.clj:18)
    clojure.test$test_var$fn__9737.invoke (test.clj:717)
    clojure.test$test_var.invokeStatic (test.clj:717)
    clojure.test$test_var.invoke (test.clj:708)
    clojure.test$test_vars$fn__9763$fn__9768.invoke (test.clj:735)
    clojure.test$default_fixture.invokeStatic (test.clj:687)
    clojure.test$default_fixture.invoke (test.clj:683)
    clojure.test$test_vars$fn__9763.invoke (test.clj:735)
    clojure.test$default_fixture.invokeStatic (test.clj:687)
    clojure.test$default_fixture.invoke (test.clj:683)
    clojure.test$test_vars.invokeStatic (test.clj:731)
    clojure.test$test_all_vars.invokeStatic (test.clj:737)
    clojure.test$test_ns.invokeStatic (test.clj:758)
    clojure.test$test_ns.invoke (test.clj:743)
    user$eval224$fn__345.invoke (form-init6789831084988863437.clj:1)
    clojure.lang.AFn.applyToHelper (AFn.java:156)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.core$apply.invokeStatic (core.clj:667)
    clojure.core$apply.invoke (core.clj:660)
    leiningen.core.injected$compose_hooks$fn__154.doInvoke (form-init6789831084988863437.clj:1)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.core$apply.invokeStatic (core.clj:665)
    clojure.core$apply.invoke (core.clj:660)
    leiningen.core.injected$run_hooks.invokeStatic (form-init6789831084988863437.clj:1)
    leiningen.core.injected$run_hooks.invoke (form-init6789831084988863437.clj:1)
    leiningen.core.injected$prepare_for_hooks$fn__159$fn__160.doInvoke (form-init6789831084988863437.clj:1)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.lang.AFunction$1.doInvoke (AFunction.java:31)
    clojure.lang.RestFn.invoke (RestFn.java:408)
    clojure.core$map$fn__5866.invoke (core.clj:2755)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:51)
    clojure.lang.Cons.next (Cons.java:39)
    clojure.lang.RT.boundedLength (RT.java:1792)
    clojure.lang.RestFn.applyTo (RestFn.java:130)
    clojure.core$apply.invokeStatic (core.clj:667)
    clojure.test$run_tests.invokeStatic (test.clj:768)
    clojure.test$run_tests.doInvoke (test.clj:768)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.core$apply.invokeStatic (core.clj:665)
    clojure.core$apply.invoke (core.clj:660)
    user$eval224$fn__357$fn__410.invoke (form-init6789831084988863437.clj:1)
    user$eval224$fn__357$fn__358.invoke (form-init6789831084988863437.clj:1)
    user$eval224$fn__357.invoke (form-init6789831084988863437.clj:1)
    user$eval224.invokeStatic (form-init6789831084988863437.clj:1)
    user$eval224.invoke (form-init6789831084988863437.clj:1)
    clojure.lang.Compiler.eval (Compiler.java:7177)
    clojure.lang.Compiler.eval (Compiler.java:7167)
    clojure.lang.Compiler.load (Compiler.java:7636)
    clojure.lang.Compiler.loadFile (Compiler.java:7574)
    clojure.main$load_script.invokeStatic (main.clj:475)
    clojure.main$init_opt.invokeStatic (main.clj:477)
    clojure.main$init_opt.invoke (main.clj:477)
    clojure.main$initialize.invokeStatic (main.clj:508)
    clojure.main$null_opt.invokeStatic (main.clj:542)
    clojure.main$null_opt.invoke (main.clj:539)
    clojure.main$main.invokeStatic (main.clj:664)
    clojure.main$main.doInvoke (main.clj:616)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.lang.Var.applyTo (Var.java:705)
    clojure.main.main (main.java:40)

pedrorgirardi commented 5 years ago

What do we lose if we only use MemoryAsset/create with a JSON encoded string metadata?

(^Asset [meta data]
   (let [byte-data (to-bytes data)]
     (if (string? meta) 
       (MemoryAsset/create byte-data ^String meta)
       (let [^java.util.Map meta-map (stringify-keys meta)] 
         (MemoryAsset/create byte-data meta-map )))))

So, always use this version (MemoryAsset/create byte-data ^String meta) instead?

mikera commented 5 years ago

I think it is helpful to do stringify-keys whenever we pass from Clojure to Java. We probably shouldn't support keyword values.

starfish-java does some default metadata creation if you don't pass an exact String as metadata, which I think is useful.

pedrorgirardi commented 5 years ago

Thanks @mikera .

We probably shouldn't support keyword values.

Is the current behaviour acceptable then? Starfish Java & Clojure won't handle "bad" metadata; it's a user error if the metadata can't be encoded as JSON.

May we close this issue (if you @mikera agree the current behaviour is correct)?

mikera commented 5 years ago

Yes I think current behaviour is correct. Exceptions should typically be thrown if the user makes an error, so that they know about it an can correct it.

Good to add (negative) tests to verify this of course.

pedrorgirardi commented 5 years ago

Cool! I will leave this open until I add tests.

datacraft-dsc / starfish-clj

Register asset with keyword metadata fails #22