Better control and allow inclusion of the _etag field

[smyrman]

As discussed in #157, there should be a consistent way to control and include _etag in embedded items as well as root items in all GET views.

[smyrman]

@Dragomir-Ivanov , @rs - to continue the discussion here. We have discussed two options previously:

1. Allow _etag to be specified in fields.
2. Allow a separate flag, e.g. etag=1 to include all E-tags.

1 is maybe what gives me the best feeling; you are planning to update an (embedded) or list item, and you ask for at least the "id" and the "_etag" field. Also it's included as a field, so why not ask for it as a field? However, I have until now assumed that 1 is maybe hard to implement.

If we talk implementation, I have a plausible way forward for 1, but we need to add one more feature to make it work: we need to allow incllusion of hidden fields (and maybe add a new Secret field to allow completely hiding). With this done, we add the fieldit self in the Index during a Resource/Index Bind

[smyrman]

Writing on my phone so wikl correct the grammar later...

[rs]

I'm all for 1., moving the _tag in the payload and using a hidden/secret/whatever field type to make it not part of the default payload.

[Dragomir-Ivanov]

This should govern inclusion the _etag into itemList case as well.

[smyrman]

Some code links.

Allow hidden fields to be included (could be first PR)

Probably around here... https://github.com/rs/rest-layer/blob/3ffe90e45fe2868e3db8dd03c92d0729e9ddeea0/schema/query/projection_evaluator.go#L62

To allow the original use-case of Hidden, we can add a new Secret bool option to schema.Field. @rs, what is the original use-case of the hidden field? Something like A users password or private key that can e.g. be changed but never shown?

Allow _etag to be specified in as a field (could be second PR)

In resource.Resource.Bind(), you can see how Connection is added to r.validator.fallback.Fields. We will need to do the same for "_etag", and set the field as ReadOnly: true, Hidden: true https://github.com/rs/rest-layer/blob/3ffe90e45fe2868e3db8dd03c92d0729e9ddeea0/resource/resource.go#L134

We need to do the same in new called by resource.index.Bind(). Note that the new function overrides the built-in Go new function which is a bit confusing -- feel free to rename new to newResource for better readability PS! resource.New would have been find for a public function, but this is private. https://github.com/rs/rest-layer/blob/3ffe90e45fe2868e3db8dd03c92d0729e9ddeea0/resource/index.go#L49 https://github.com/rs/rest-layer/blob/3ffe90e45fe2868e3db8dd03c92d0729e9ddeea0/resource/resource.go#L78

There probably also need to be some more changes to actually put the E-tag into the item payload before schema/query applies the projection....

[Dragomir-Ivanov]

I think that Hidden have to be hidden by default, but can be requested via fields= query parameter. This will be inconvenient because in order to get a hidden field, one needs to enumerate all non-hidden fields needed. That is why I wanted this * field matcher.

Currently Hidden can also be written when supplied in PATCH. I think that is okay.

The new Secret field would not be requested in any way, and will not be available for writes. Now if we don't specify a field in the resource schema, it will not be available for writes, but will be returned upon GET. Secret will prevent this.

Your comments appreciated.

[smyrman]

Some minor points.

This will be inconvenient because in order to get a hidden field, one needs to enumerate all non-hidden fields needed. That is why I wanted this * field matcher.

Agree, but I think it is still good enough. We can have another look at * later. I think the best approach for * is actually to replace it (in-place) with all non-hidden fields from the schema before even validating fields, and then document the last mention wins rule.

[Secret field] and will not be available for writes

This should be decided by the existing ReadOnly field. There could be valid use-cases for writing a field without being able to read/expos it.

[smyrman]

Maybe a better name then Secret is WriteOnly? It becomes a bit funny (although perfectly valid) if you set both ReadOnly and WriteOnly, but exept for that it might be a better name. Those are also well-understood concepts in programming...

[smyrman]

Another option could be to replace the ReadOnly field with an enum:

type FieldMode

const (
    ModeReadWrite = iota
    ModeReadOnly
    ModeWriteOnly
    ModeNoAcces
)

[Dragomir-Ivanov]

@smyrman I actually like the last idea about the FieldMode. Don't you like to make it just mask:

type FieldMode uint
const (
    ModeRead = iota
    ModeWrite
)

By default fields can have ModeRead | ModeWrite, but then user can overwrite it. This will make Hidden type redundant as well.

@rs Your input appreciated.

[rs]

Either way works for me. My preference would go for @smyrman's version as it is more expressive.

[smyrman]

By default fields can have ModeRead | ModeWrite

Yeah, I thought about that. However, since it's Go, one extra consideration to have in mind is that all types have default zero values. Therefore, if you can get away with make the default case a zero value, the code get's signifincantly less complex as no struct initialization method is needed.

If you flip your proposal, and we have MaskNoWrite = 0x01, ModeNoRead = 0x02, that would be bitwise equicalent to the iota constants, although perhaps a bit hard to read.

const(
    ModeReadWrite = 0x00
    ModeReadOnly = 0x01 // same as ModeNoWrite
    ModeWriteOnly = 0x02 // same as ModeNoRead
    ModeNoAcces  = 0x03 // same as ModeNoWrite | ModeNoRead

This will make Hidden type redundant as well.

How so? Are we no planning to "redefine" hidden to mean weather or not a field is hidden from the response by default? Won't we still need a field for that?

We could of-course define a bitmask for that as well, but the main goal off the schema should probably be to optimize for the best expressiveness.

[smyrman]

Otherwise it was a good proposal @Dragomir-Ivanov.

[Dragomir-Ivanov]

@smyrman Yes, that makes sense. I was mistaken, this work will not depreciate Hidden.