Able to set encoding to query value

[evshary]

Currently, PutBuilder supports encoding

let putbuilder = self
            .session
            .put(zenoh_key, buf)
            .encoding(Encoding::Exact(KnownEncoding::AppCustom));

But now get doesn't support encoding, so we need to create value first

let value = Value::new(buf.into()).encoding(Encoding::Exact(KnownEncoding::AppCustom));
let getbuilder = self
            .session
            .get(&zenoh_key)
            .with_value(value);

It might be easier and cleaner for users to use get with encoding support.

let getbuilder = self
            .session
            .get(&zenoh_key)
            .with_value(buf)
            .encoding(Encoding::Exact(KnownEncoding::AppCustom));

[Mallets]

Marking this PR for additional consideration in #666 and #665 .

[milyin]

If user miss the order of operations and set encoding before value, current code just silently ignores encoding operation. This doesn't look right.

Unlike in PutBuilder, the value is optional in get operation. I'd propose make this method create new empty value if value was not set. In this case this code will be valid:

let getbuilder = self
            .session
            .get(&zenoh_key)
            .encoding(Encoding::Exact(KnownEncoding::AppCustom));

This still might be confusing, that the code below will lose the encoding set, but on the other hand this is how builders works: new value overrides old one.

let getbuilder = self
            .session
            .get(&zenoh_key)
            .encoding(Encoding::Exact(KnownEncoding::AppCustom))
            .with_value(buf)

OF course this should be specified in the documentation, that encoding sets the encoding of exinsting value or creates emply vlaue if value was not set.

[milyin]

see above: the problem that the code does nothing if optional value is not set. But value may be added later and this will be confusing that encoding operation did nothing

[evshary]

see above: the problem that the code does nothing if optional value is not set. But value may be added later and this will be confusing that encoding operation did nothing

Good idea. I've added some changes now.

[Mallets]

I'm still not convinced about the proposed changes. Let me take the problem for a more broad perspective.

By definition a Value is tuple of (payload, encoding).
with_value(...) allows to attach a Value to a Query. That means that whatever is passed as Value in the GetBuilder is found exactly the same in the Query passed to the queryable.

At this point one may argue that this symmetry is not reflected in the PutBuilder and the Sample received by the subscriber. However, the main difference is that a put takes a mandatory Value while get doesn't. Therefore, encoding is guaranteed to always operate on a existing Value. With the proposed changes the guarantee is not upheld. Moreover, an encoding without having set an explicit value will have a side effect to create an empty value, which I believe is not a nice behaviour.

To provide the sought after guarantee, the with_value should hence return a second builder that provides a encoding method. In this way, it is not possible to set the encoding without having set the value first. This however requires a bit more changes than the ones proposed in this PR.

[milyin]

I tried to restrict setting encoding by adding new builder type (in fact by parametrizing existing builder, but that's the same thing). The result is not satisfying. Look at this piece of code:

       let mut query = req.state().0.get(&selector).consolidation(consolidation);
        if !body.is_empty() {
            let encoding: Encoding = req
                .content_type()
                .map(|m| m.to_string().into())
                .unwrap_or_default();
            query = query.with_value(Value::from(body).encoding(encoding));
        }

With different types for builders it compiles with error

error[E0308]: mismatched types
   --> plugins/zenoh-plugin-rest/src/lib.rs:409:21
    |
409 |             query = query.with_value(Value::from(body).encoding(encoding));
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `false`, found `true`
    |
    = note: expected struct `zenoh::query::GetBuilder<'_, '_, _, false>`
               found struct `zenoh::query::GetBuilder<'_, '_, _, true>`

This is expected: we can't anymore update builder differently depending on incoming parameters.
So I think the approach with multiple builder types should be used very carefully.

[p-avital]

My opinion on this is that:

• Encodings in general don't provide a lot of value, with the consequences that:
  • Encodings API being slightly less comfy is tolerable;
  • More importantly, Encodings should never make higher priority API worse.
• I'm a firm believer of the "path of least surprise", so I'm against an builder method that can be called before a value's been added:
  • If we ignore calls until a value is added, the lack of reversability of the call is surprising.
  • If we create an empty value, we're creating something that might not respect the advertised encoding (JSON) for example, causing surprises at a distance.
  • If we store the latest encoding and apply it to whatever value is being added next, we might overwrite an encoding that might have been explicitly set, causing surprise (and arguably, which one of the encodings should triumph in that situation is debatable).
• With @milyin's highlight, we can see that a GetWithValue would break the (rather common) pattern of conditionally adding options to a builder. If the only added value of a GetWithValue is that we can have a method to set the (to my eyes unimportant) encodings, I'd rather we not have that method and keep the single type builder.

TL;DR: I'm in favour of not having a builder.with_encoding() method at all, despite the "asymmetry" with put.

[milyin]

We may follow this pattern in all builder methods which accepts complex objects. Maybe it can be convenient:

let getbuilder = self
            .session
            .get(&zenoh_key)
            .with_build_value() // switch to builder for Value
            .payload("Foo")
            .encoding(Encoding::Exact(KnownEncoding::AppCustom));
            .res_with() // return to original builder
            .timeout(100)
            .res()?;

This approach allows to use chain of calls specific for parameter type inside the chain of calls of main object.
It's questionable, is this really simplifies our API or it's just useless sugar.
But this is the logical result of following the path where we add settings of internal values to outer object.

[milyin]

Linked it in issue #716 to continue discussion, closing this issue