eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 783 forks source link

Missing response for newly created rules without UIDs #1047

Closed adimova closed 8 years ago

adimova commented 8 years ago

When someone creates a rule without providing the UID using the RuleRegistry or REST, he will not receive the auto-genereted UID

danchom commented 8 years ago

I think this is not a bug, it is architectural problem. The Rule class does not have a setUID() method. The uid can be set through the constructor only. In the RuleEngine the addRule() returns a new added rule object (which has uid property), but its caller the RuleRegistry.add(Rule) which is inherited from Registry interface does not return added object. May be Rule.setUID() method has to be added? The other way the uid can be updated through reflection, but I don't like to use reflection.

adimova commented 8 years ago

Can we extend the functionality of RuleRegistry with method requestUniqueID() ?

kaikreuzer commented 8 years ago

I would also be open to discuss whether Registry.add() should return the added element.

adimova commented 8 years ago

yes, fixing the return type of the Registry.add() method will be the best solution, but all implementations of the Registry interface will need to be fixed then

kaikreuzer commented 8 years ago

I think almost all of them extend AbstractRegistry, so we can change it in there.

danchom commented 8 years ago

For me it is best solution too. Also to return a value for a method which was void, it is not backward incompatible change.

marinmitev commented 8 years ago

I also like the idea to return the added rule

danchom commented 8 years ago

The returned rule may have additional changes. For example if the added rule is specified by templateUID, the returned rule will have triggers, conditions and actions in case when the template is available in the rule engine

adimova commented 8 years ago

So, you propose REST to return the rule object, not only the UID

kaikreuzer commented 8 years ago

In a proper REST style, it should actually return the url of the created element (which in this case will contain the UID as well).

danchom commented 8 years ago

I'm not sure about to change the REST API. I think the UID is enough. In the most cases the rule object will not be needed.

adimova commented 8 years ago

last to clarify, which will be the return type of add(), key or element?

kaikreuzer commented 8 years ago

From http://www.ietf.org/rfc/rfc2616.txt:

   The Location response-header field is used to redirect the recipient
   to a location other than the Request-URI for completion of the
   request or identification of a new resource. For 201 (Created)
   responses, the Location is that of the new resource which was created
   by the request.

So we should imho definitely fill the location header field. The body of the response can imho remain empty. Alternatively, I would vote for returning the full rule entity back.

which will be the return type of add(), key or element?

The element as we do not even know what the key is within the AbstractRegistry.

adimova commented 8 years ago

i'm ok with that

kaikreuzer commented 8 years ago

Any volunteer to create a PR for this?

adimova commented 8 years ago

I will do the fix

danchom commented 8 years ago

What will be the fix here? 1) Is the Registry.add(E) method will be changed to return a value (an added element)? 2) fill the location header of response of create rest request? or both?

kaikreuzer commented 8 years ago

Imho both, because we need 2 and this cannot be done without 1.

danchom commented 8 years ago

+1