Navigation Menu

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

Implemented support for custom serialization #2492

Merged
merged 1 commit into from Nov 26, 2020

Conversation

ferencdg
Copy link
Contributor

@ferencdg ferencdg commented Nov 9, 2020

Added @ResultSerializer UDA to control serialization

@ferencdg
Copy link
Contributor Author

ferencdg commented Nov 9, 2020

We needed this functionality to interact with non-vibe.d clients that expected the serialized return value to be in a specific format. For example Prometheus server would call into our vibe.d REST service and we need to return the result in a specific(non-json)
format.

@ferencdg ferencdg force-pushed the master branch 5 times, most recently from 599c8f1 to 487c958 Compare November 10, 2020 08:39
Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

Thanks for this effort! I wanted to do this for a long time, too, but so far didn't have an important enough use case to actually do it. There are a few details that should be changed, but in general the changes look good.

web/vibe/web/common.d Outdated Show resolved Hide resolved
web/vibe/web/common.d Outdated Show resolved Hide resolved
package alias DefaultSerializerT (FuncRetT) =
ResultSerializer!(
function (FuncRetT rev_val){
return rev_val.serializeToJsonString();
Copy link
Member

Choose a reason for hiding this comment

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

I know the existing implementation isn't there right now either, but since this introduces a new API, it should be made range based instead of requiring the full result to be stored in memory - the appropriate signature would be (ref output_range, ref FuncRetT v) or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good idea, and I will change the interface to accept a new output parameter, however I don't plan to restrict the interface to only accept OutputRanges. The client of the serializiation code might decide to use a custom output interface other than the OutputRange and the same client might also provide serialization UDAs for that specific custom output interface. Would there be any benefit restricting the type?

Copy link
Member

Choose a reason for hiding this comment

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

Since the web/common.d code provides an object that needs to work for all user serializers, don't really see how to make this work without some form of common interface, and an output range is just the most simple form of that. But of course, since this would be a template parameter, the operations that the concrete type supports are not restricted, and user code could still make use of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could check those UDA's in the getInterfaceValidationError method, but I would need to iterate through all the functions and all the UDA's and then check a certain parameter. Would you prefer that?

Copy link
Member

Choose a reason for hiding this comment

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

I don't even think that we need any explicit checks, just documenting it for @resultSerializer should be enough (would also be nice to have very a brief unittest example there).

function (FuncRetT rev_val){
return rev_val.serializeToJsonString();
},
function (string serialized){
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, (input_range) with isInputRange!(typeof(input_range)) being guaranteed by the caller would be the way to go here, possibly using a ref output parameter for the result instead of returning it to make it easier to efficiently parse large structs without extra copies, although this shouldn't be a serious issue.

web/vibe/web/common.d Outdated Show resolved Hide resolved
web/vibe/web/common.d Outdated Show resolved Hide resolved
@ferencdg
Copy link
Contributor Author

ferencdg commented Nov 16, 2020

thanks for the comments @s-ludwig, I changed most of them, please see my replies.

I also have 2 other serialization related pull request:
#2493
#2495

web/vibe/web/common.d Outdated Show resolved Hide resolved
private {
Json m_jsonResult;
}

@safe:

///
this (int status, string result, string file = __FILE__, int line = __LINE__, Throwable next = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any attributes that can go here, such as@safe and nothrow ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@safe already applied (@safe:) and other cannot go, but I will change the attributes on jsonResult in a separate commit

web/vibe/web/common.d Outdated Show resolved Hide resolved
@@ -711,7 +755,7 @@ class RestInterfaceClient(I) : I
* Returns:
* The Json object returned by the request
*/
Json request(HTTPMethod verb, string name,
string request(HTTPMethod verb, string name,
Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch that this also is a breaking change due to the protected atttribute. I'm also not sure why it is protected in the first place, though, so we might just deprecate it and add the new signature as a private method (and go for a string output range instead of a string return value to prepare for dropping appender in favor of a StreamOutputRange or similar).

The old deprecated method would then only be called if no custom serialization is in effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, just let quickly ask, if I understood correctly:

  1. we lwill eave the original protected 'request' method in place, and we will implement it by using a call into our new private 'request' method
  2. the new private 'request' method will no longer return a string, but it will accept a new parameter of type StreamOutputRange

is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

For number two I was thinking of a generic output range argument, but other than that that sounds correct.

The only issue is that the protected request() will have to re-parse the result to Json if it just calls the private one. So maybe we can move the serialization independent code to a separate method that is called by both and then let the old request just do its serialization the old way, instead of calling the new private one.

Copy link
Contributor Author

@ferencdg ferencdg Nov 17, 2020

Choose a reason for hiding this comment

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

I was just thinking and I think we should pass an InputRange(or similar) to the deserializer. So shouldn't we return HTTPClientResponse from the private 'request' method? and later we can directly pass the InputStream from the HTTPClientResponse to the deserializer.

Copy link
Contributor Author

@ferencdg ferencdg Nov 17, 2020

Choose a reason for hiding this comment

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

I see there is an InputRangeStream already in vibe.d so I could 'convert' InterfaceProxy!InputStream and pass directly to the serializers

function (ref output_stream, ref value){
output_stream ~= serializeToJsonString(value);
},
function (input_stream){
Copy link
Contributor Author

@ferencdg ferencdg Nov 18, 2020

Choose a reason for hiding this comment

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

I was unable to extract the actual stream object from the ProxyInterface as it currently requires knowing the type of the stream object compile time, but it is only decided in runtime. As a result of this, currently the type of the 'input stream' is ProxyInterface!InputStream.

StreamInputRange also didn't work thanks to the ProxyInterface

Even if I were somehow be able to to get StreamInputRange to work, the code in the json serializer requires a ForwardRange not just a simple InputRange

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that both of those were the case - StreamInputRange only accepting InputRange and the JSON parser code requiring a forward range (for skipNumber only as it seems). I'll see what I can do there.

@ferencdg
Copy link
Contributor Author

I think I made all the changes @Geod24 @s-ludwig, please let me know if I should change anything else

package alias DefaultSerializerT (FuncRetT) =
ResultSerializer!(
function (ref output_stream, ref value){
output_stream ~= serializeToJsonString(value);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
output_stream ~= serializeToJsonString(value);
serializeToJson(output_stream, value);

This can now use the range overload of serializeToJson to avoid the temporary copy. The parameter should be named output_range, tough, as this relies on a range interface rather than a stream one (same for input_stream below). It might make sense to go for a stream interface for certain serializers and large responses, but for now I think the range interface is more practical.

Copy link
Contributor Author

@ferencdg ferencdg Nov 24, 2020

Choose a reason for hiding this comment

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

thanks makes sense for the output, however I couldn't get rid of using streams for the input, please see:
https://github.com/vibe-d/vibe.d/pull/2492/files#r526105742

I could create an input range from the stream by copying it, but this would be inefficient

immutable serializer_ind = get_matching_content_type!(result_serializers)(content_type);
foreach (i, serializer; result_serializers)
if (serializer_ind == i)
return serializer.deserialize(ret.bodyReader);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return serializer.deserialize(ret.bodyReader);
{
auto rng = streamInputRange(ret.bodyReader);
return serializer.deserialize(rng);
}

This should provide a range interface to the serializer to be consistent with serialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should provide a range interface to the serializer to be consistent with serialize.

please see: https://github.com/vibe-d/vibe.d/pull/2492/files#r526105742

Copy link
Contributor Author

@ferencdg ferencdg Nov 26, 2020

Choose a reason for hiding this comment

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

This should provide a range interface to the serializer to be consistent with serialize.

please see: https://github.com/vibe-d/vibe.d/pull/2492/files#r526105742

@s-ludwig for the follow up PR, are you okay with my suggestion?
"I could create an input range from the stream by copying it, but this would be inefficient"

I could call the readAllUTF8 function on the stream before I call the deserialize function, but that would be very inefficient for the memory. If I do that we could have InputRage(ForwardRange) easily. Actually I am aleady calling readAllUTF8 anyway inside the deserialize function...

I think in long term the best thing for performace would be to

  1. get the stream out of the proxy
    2a. write a wrapper around it so the stream can be a 'ForwardRange' - I don't think this is easy to do OR
    2b change the JsonStringSerializer not to require 'ForwardRange'

else requestHTTP(url, reqdg, resdg);
HTTPClientResponse client_res;
if (http_settings) client_res = requestHTTP(url, reqdg, http_settings);
else client_res = requestHTTP(url, reqdg);
Copy link
Member

Choose a reason for hiding this comment

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

Just as a note, this degrades performance somewhat, because it currently causes the response object to be allocated on the heap instead of the stack, but this this will be a non-issue once the implementation is changed to vibe-http.

Copy link
Contributor

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

LGTM

/// The HTTP status code
@property const(Json) jsonResult() const { return m_jsonResult; }
/// The result text reported to the client
@property const(Json) jsonResult () const nothrow pure @safe @nogc
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@property const(Json) jsonResult () const nothrow pure @safe @nogc
@property inout(Json) jsonResult () inout nothrow pure @safe @nogc

Comment on lines 508 to 511
function (ref output_range, ref value){
serializeToJson(output_range, value);
},
function (input_stream){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function (ref output_range, ref value){
serializeToJson(output_range, value);
},
function (input_stream){
function (ref output_range, ref value) {
serializeToJson(output_range, value);
},
function (input_stream) {

Style nits

Comment on lines 163 to 170
function (output_stream, test_struct){
output_stream ~= serializeToJsonString(test_struct);
},
// input_stream implements InputStream
function (input_stream){
return deserializeJson!TestStruct(input_stream.readAllUTF8());
},
"application/json")()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same style nits

Comment on lines +1965 to +1982
// Get required headers - Don't throw yet
string[] missingKeys;
foreach (k, ref v; reqReturnHdrs.byKeyValue)
if (auto ptr = k in client_res.headers)
v = (*ptr).idup;
else
missingKeys ~= k;

// Get optional headers
foreach (k, ref v; optReturnHdrs.byKeyValue)
if (auto ptr = k in client_res.headers)
v = (*ptr).idup;
else
v = null;
if (missingKeys.length)
throw new Exception(
"REST interface mismatch: Missing required header field(s): "
~ missingKeys.to!string);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for the future: I'd really like us to improve this part so it doesn't allocate this much...

Added @resultSerializer UDA to control serialization
@dlang-bot dlang-bot merged commit d4e7971 into vibe-d:master Nov 26, 2020
@s-ludwig
Copy link
Member

Now dlang-bot has merged without a merge commit again ;)

Also, my comments w.r.t input stream vs. input range were not yet resolved. I'll try to prepare a follow-up PR for the recent changes ASAP.

@Geod24
Copy link
Contributor

Geod24 commented Nov 26, 2020

Now dlang-bot has merged without a merge commit again ;)

My bad... We probably shouldn't use dlang-bot for single-commit PRs then. Or perhaps make it configurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants