org-arl / fjage

Framework for Java and Groovy Agents
https://fjage.readthedocs.io/en/latest/
Other
26 stars 13 forks source link

Handle named parameters more consistently #292

Open ettersi opened 10 months ago

ettersi commented 10 months ago

The current behaviour of named parameters is quite confusing.

This parameter lookup succeeds.

groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> 42

This parameter lookup also succeeds.

groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> 42

This one fails.

groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("spam.foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> null

Finally, the last lookup would have succeeded if I had instead done this.

package spam;

enum foo implements org.arl.fjage.param.Parameter { bar }

/////////////////////////////////////////////////////

rsp = new ParameterRsp(new ParameterReq())
rsp.set(spam.foo.bar, 42, true)
rsp.get(new NamedParameter("bar"))

The difference between the last two lookups is particularly problematic because someAgentId.get(new NamedParameter("bar")) internally switches between the two depending on whether the bar parameter is defined as a Java object somewhere in the classpath. Therefore, pretty harmless-looking Fjage code may produce different results without throwing an error depending on whether it is run using java XXX -cp A or java XXX -cp B.

mchitre commented 10 months ago
groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("spam.foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> null

failing is clearly a bug, if this works:

groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> 42
notthetup commented 10 months ago

Something in

https://github.com/org-arl/fjage/blob/0d4bc3ee147e478d5c4c990b3c3088c0ae246c9b/src/main/java/org/arl/fjage/param/ParameterRsp.java#L151-L161

or

https://github.com/org-arl/fjage/blob/0d4bc3ee147e478d5c4c990b3c3088c0ae246c9b/src/main/java/org/arl/fjage/param/NamedParameter.java#L82-L92

ettersi commented 10 months ago
groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("spam.foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> null

failing is clearly a bug, if this works:

groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> 42

I thought this was by design? https://github.com/org-arl/fjage/pull/285#discussion_r1286457171

I believe the intention here was that NodeInfo.location should be reduced to location, but org.arl.unet.nodeinfo.NodeInfoParam.location should not be reduced to location.

mchitre commented 10 months ago

Not quite by design, but a residue from some fix.

As noted in the comment there: "We don't use qualified named parameters". That's where this problem comes from, since this issue deals with qualified parameter names, which aren't supported. We could, if we see a use case for them. Alternatively we can document that we don't support, and better yet, warn or throw an exception if someone defines a named parameter with qualified names?

ettersi commented 10 months ago

My objective here is to make it possible and reasonably easy to retrieve parameters from a Java gateway. AFAICT, there are two options to make this happen.

The second option sounds like less work to me, but maybe I'm missing something?

mchitre commented 10 months ago

The second option is doable today without having to use qualified names with named params. Right?

ettersi commented 10 months ago

The second option is doable today without having to use qualified names with named params. Right?

No, because of this.

groovy:000> rsp = new ParameterRsp(new ParameterReq())
===> ParameterRsp[]
groovy:000> rsp.set(new NamedParameter("spam.foo.bar"), 42, true)
===> null
groovy:000> rsp.get(new NamedParameter("bar"))
===> null

When you do e.g. node.get(new NamedParameter("location")), you get back a org.arl.unet.nodeinfo.NodeInfoParam.location, and so rsp.get(new NamedParameter("location")) on that is going to give you null unless you have org.arl.unet.nodeinfo.NodeInfoParam.location in your classpath.

mchitre commented 10 months ago

My vote for making NamedParameter work for fully-qualified parameter names, and essentially being equivalent to an Enum parameter.