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

Added value objects for possible values #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dschulten
Copy link

@@ -49,7 +49,19 @@ The media type for JSON Siren is `application/vnd.siren+json`.
"fields": [
{ "name": "orderNumber", "type": "hidden", "value": "42" },
{ "name": "productCode", "type": "text" },
{ "name": "quantity", "type": "number" }
{ "name": "quantity", "type": "number" },
{ "name": "color", "type": "radio", "title": "Choose a Color",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A name describing the control. Field names MUST be unique within the set of fields for an action. The behaviour of clients when parsing a Siren document that violates this constraint is undefined. Required.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this. The 'color' attribute was not meant to appear twice. Changed to 'accessories'.

@apsoto
Copy link

apsoto commented Jan 23, 2017

FYI I have no capability to change the spec, but IMO, I doubt the spec will be changed without 'real' implementations and feedback. Therefore I think you should move on extending it yourself and report back to the group about how it went, what was good, what was bad, etc....

In general I don't think it's a good practice to change a spec before real world implementations are tried.

@kevinswiber
Copy link
Owner

@apsoto Yes, that's the general direction I'd like to take. I've implemented this, but I'd like some more feedback from other folks.

@smizell
Copy link

smizell commented Feb 14, 2017

I wanted to drop in and give some feedback on this topic. I ended up going with this as a solution for providing multiple values for a given field, and it worked well.

My only issue was that the current proposal feels a tad out of place. All other code around modeling representations is laughably simple with almost no conditions (this is the part of Siren's design that resonates with me). In other words, it mapped almost directly to my classes. But this proposal introduces a special rule, such as when converting to JSON, if it's a string or number do this, and if it's a list of Value objects, do that. This also shows up in the JSON Schema as the only place a "oneOf" is used.

I don't say this to say it's bad at because it worked for me, however it just seems almost too special for Siren. :)


I have wondered in this if there is a different way to think about this. Instead of a FieldValueObject, you would instead have a MultiValueField. This multi-value field has a values property that is always an array of objects with title and value as properties. You would then limit the type values of each field type, as radio and checkbox would by multi-value and the others would be a normal field. This would provide some clarity in the spec and even the JSON Schema in my opinion.

For me, it would remove a little bit of the magic that I'm feeling. It would also mimic the way entities works where there are two different kinds of sub entities that rely on duck typing to determine their type when deserializing.

@smizell
Copy link

smizell commented Feb 14, 2017

Just to add one note of clarity for my suggestion, like how "Sub Entity" has "Embedded Representation" and "Embedded Link," you would have "Field" that would have "Single Value Field" and "Multi-Value Field" (or of course better names). The JSON Schema would then be an anyOf when defining the field similar to how the Sub Entity definition is done.

@dschulten
Copy link
Author

dschulten commented Nov 17, 2019

While implementing Siren support for the Insomnia ReST client, I found @smizell 's observation helpful. The suggested PR (which was built on a comment by @kevinswiber in the google group) breaks an implementation which relies on the value of a field being a scalar value (Insomnia does that by default). OTOH I just realized that this proposal already made it into the siren json schema. It appears that relying on field values to be scalar values in Siren would also not be correct based on the existing readme, which makes no such promise.

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 this pull request may close these issues.

7 participants