org-arl / fjage

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

AgentID.get(NamedParameter) does not work #284

Closed ettersi closed 1 year ago

ettersi commented 1 year ago

This works:

> req = new ParameterReq(node).get(new NamedParameter("nodeName"))
ParameterReq[nodeName:?]

> node.request(req)
ParameterRsp[nodeName:A]

This doesn't:

> node.get(new NamedParameter("nodeName"))

Based on my understanding of the code, the two should be exactly the same: https://github.com/org-arl/fjage/blob/454ca543e83c82cf7e8cfc0632c28b7a1cd55a92/src/main/java/org/arl/fjage/AgentID.java#L189-L214

ettersi commented 1 year ago

Found it! The problem is the rsp.get():

> param = new NamedParameter("nodeName")
nodeName

> req = new ParameterReq(node).get(param)
ParameterReq[nodeName:?]

> rsp = node.request(req)
ParameterRsp[nodeName:A]

> rsp.get(param) == null
true

Specifically, the problem is this line:

https://github.com/org-arl/fjage/blob/454ca543e83c82cf7e8cfc0632c28b7a1cd55a92/src/main/java/org/arl/fjage/param/ParameterRsp.java#L95

> rsp.param == param
false

> rsp.param.class
class org.arl.unet.nodeinfo.NodeInfoParam

> param.class
class org.arl.fjage.param.NamedParameter
ettersi commented 1 year ago

Logging this here so I don't forget.

@mchitre requested that a fully qualified NamedParameter should match precisely the parameter with the same fully qualified name. This will require changes beyond the change to ParameterRsp.get() mentioned above. Specifically, this line here will need to be updated:

https://github.com/org-arl/fjage/blob/454ca543e83c82cf7e8cfc0632c28b7a1cd55a92/src/main/java/org/arl/fjage/param/ParameterMessageBehavior.java#L199

mchitre commented 1 year ago

Took a deeper look at this.

The response is meant to have the properly fully qualified enum, so that behavior is correct. The idea is that if we ask a parameter with just a name, it gives us the parameter and tells us its class. In this sense, this isn't a bug.

The reason node.nodeName works is that GroovyExtensions ignores which parameter is returned when asking for a single parameter: https://github.com/org-arl/fjage/blob/454ca543e83c82cf7e8cfc0632c28b7a1cd55a92/src/main/groovy/org/arl/fjage/GroovyExtensions.groovy#L67

Matching named parameters with fully qualified enums in a get() seems like a reasonable thing to do, so we can add that as a feature. That'll allow the node.get(new NamedParameter("nodeName")) to work as expected.

mchitre commented 1 year ago

Implemented in #285