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

Optionally serializing ubyte[] as base64 string #2505

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

ferencdg
Copy link
Contributor

@ferencdg ferencdg commented Nov 30, 2020

This can save brandwidth and decrease latency of REST queries

usage:

@serPolicy!(Base64ArraySerPolicy)
interface ITestBase64
{
    @safe X getTest();
}

@ferencdg
Copy link
Contributor Author

this is an other serialization change
@s-ludwig @Geod24 please let me know if anything should be changed

@s-ludwig
Copy link
Member

s-ludwig commented Dec 1, 2020

I wonder why the policy logic needs to be repeated in the web framework. Couldn't you just use the @resultSerializer attribute to specify a serializer that applies a certain policy?

@ferencdg
Copy link
Contributor Author

ferencdg commented Dec 1, 2020

I wonder why the policy logic needs to be repeated in the web framework. Couldn't you just use the @resultSerializer attribute to specify a serializer that applies a certain policy?

  1. I wanted to have an easy way to specify the policy for ALL interface methods, so I declared it on the interface. ResultSerializers are attached to interface methods, and I would find it tedious to create REST interfaces and attach resultSerializer to all methods just to add a new policy. When I added the resultSerializers 2 weeks ago, I attached them to the methods and not to the interfaces. I was afraid that if I attach the resultSerialized to the interface, then one generic templated method might not be enough to serialize the result of ALL methods.
  2. Result serializer only serializes the results, but when we define a policy on the interface I imagined that it will affect the serialization of everything: result, query parameters, headers...(although currently the policy only affects the return value...)

@ferencdg
Copy link
Contributor Author

ferencdg commented Dec 1, 2020

I wonder why the policy logic needs to be repeated in the web framework. Couldn't you just use the @resultSerializer attribute to specify a serializer that applies a certain policy?

  1. I wanted to have an easy way to specify the policy for ALL interface methods, so I declared it on the interface. ResultSerializers are attached to interface methods, I would find it tedious to create REST interfaces and attach resultSerializer to all methods just to add a new policy.
  2. Result serializer only serializes the results, but when we define a policy on the interface I imagined that it will affect the serialization of everything: result, query parameters, headers...(although currently the policy only affects the return value...)

@s-ludwig please let me know if you would like to see a different design

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, just the one nit on the name, and I think this could use some documentation.

}

///
public alias serPolicy (Args...) = SerPolicy!(Args);
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
public alias serPolicy (Args...) = SerPolicy!(Args);
public alias serializationPolicy (Args...) = SerializationPolicy!(Args);

It's better to be a little more verbose in the name for this public-facing method.

@Geod24
Copy link
Contributor

Geod24 commented Dec 29, 2020

I did a few modifications:

  • Fixed the documentation (indent was wrong, and the section name was wrong too, it should have been Serialization_Policies: instead of Serialization Policies: to count as a section name);
  • Renamed Base64ArraySerPolicy to simply Base64ArrayPolicy, since it's already in the serialization package;
  • Slightly rephrased the documentation and extended it to say it applies to all types;

@ferencdg : Sorry I missed it earlier, but why is this only on return values ? It should also apply to parameters. In fact, the issue was noticed on a parameter (when accepting an SCPEnvelope).

This can save brandwidth and decrease latency of REST queries.
@Geod24 Geod24 merged commit 32746c6 into vibe-d:master Jan 5, 2021
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.

None yet

3 participants