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

Fix #1541 - Optional attribute enhancement #1650

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

Conversation

tchaloupka
Copy link
Contributor

Lets try this. Hope its reasonably simple change.
I went with the @optional!In / Out proposal mentioned in the enhancement to maintain backward compatibility. So @optional works as optional for deserialization and serialization.
@optional!In is optional just for deserialization and @optional!Out is optional for serialization.

There is also @optional!InOut which is the same as just @optional

Added some unittests which passes fine.
Also tried this with our codebase and it can really save some data (more than 50% in our case) so its pretty usefull.

Thanks @etcimon for pointing out the right place to look at.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Lets try this. Hope its reasonably simple change.
I went with the @optional!In / Out proposal mentioned in the enhancement to maintain backward

I like the idea and I think we should also provide the according @ignore!In, @ignore!Out.

@s-ludwig
Copy link
Member

s-ludwig commented Feb 9, 2017

What I wonder about the chosen representation is if @optional!Out actually makes sense in practice, since it would not be possible in general to deserialize serialized data in that case, which means @optional!InOut is always the better choice. My approach would probably have been binary: a) optional only for deserialize b) optional for both

That would also reduce the pressure for global symbols with generic names at bit (In/Out/InOut). Maybe there is even a nice way to write this, without using global aliases then...

@s-ludwig
Copy link
Member

s-ludwig commented Feb 9, 2017

I like the idea and I think we should also provide the according @ignore!In, @ignore!Out.

SImilarly, only @ignore!In and @ignore!InOut really make sense.

@zunkree
Copy link

zunkree commented Feb 27, 2017

I believe @optional!In, @optional!Out and @optional!InOut is more flexible way to support optional fields during serialization/deserialization. The same with @ignore.

@s-ludwig
Copy link
Member

So what would be the use case for @optional!Out?

@tchaloupka
Copy link
Contributor Author

What about for example some nullable field from the DB?
I'll do the ignore attribute same way after this will be decided which way to go.

Also I don't like In, Out, InOut aliases too much, but can't figure out the better way. Ideas are welcome.

@eresid
Copy link

eresid commented Mar 9, 2017

Gson library has @Expose annotation with boolean parameters: serialize and deserialize - looks worse than @optional!(In/Out/InOut). So as for me, your variant better.

@s-ludwig b) optional for both

This is the best option, as in most cases we have use a same logic, will be less code

@tchaloupka
Copy link
Contributor Author

I've added possibility to set direction also to IgnoreAttribute.

@eresid
Copy link

eresid commented Mar 23, 2017

Please, could you merge it with master? This is need for my firebase cloud messaging library

@s-ludwig
Copy link
Member

So what if we simply add an additional attribute @lean that is the same as @optional!InOut is now? It circumvents the @optional!Out case and avoids the global In and InOut symbols.

@eresid
Copy link

eresid commented Mar 23, 2017

@s-ludwig why not, or @unrequired :)

@tchaloupka
Copy link
Contributor Author

@s-ludwig so now it is:

  • @optional!InOut stays @optional
  • @ignore!InOut stays @ignore
  • @optional!Out and @ignore!Out goes away as not needed
  • @optional!In became @lean

But what about @ignore!In? It can't be the same as @optional!In as it shouldn't set the field at all. Although I'm not sure if there is a case to use @ignore!In.

@zunkree
Copy link

zunkree commented Apr 7, 2017

@tchaloupka maybe @skip is okay for @ignore!In, what do you think?

@s-ludwig
Copy link
Member

s-ludwig commented Apr 7, 2017

The issue with @ignore!In is true indeed. I thought about that after re-reading the thread, but got distracted before coming to a conclusion. It seems like finding separate words for the different directions scratches the language boundaries. Is there maybe an existing serialization library that offers the same functionality?

I have the feeling that maybe the whole @ignore customization is better done using a serialization policy rather than in a global fashion:

struct APIPolicy {}
struct S {
    @ignore!APIPolicy
    string password;
}

DB serialization would work without specifying a policy and API queries would be served with serializeWithPolicy!(JsonStringSerializer, APIPolicy). This would keep things working in both directions when using the same policy and also possibly makes the intent of the attribute more obvious.

Of course, serialization policies need to be embraced by the code base (e.g. REST/web modules) before this is a generally applicable solution.

@tchaloupka
Copy link
Contributor Author

And what about leaving @optional and @ignore without specialization as it was and then introduce for example @serializePolicy and @deserializePolicy to the REST/web interfaces and use them to fine tune serialization?

So for example with the REST API, it could be:

//define default API policy
@serializePolicy!MyPolicy @deserializePolicy!MyPolicy
interface IFoo
{
    Bar[] getBars(); // use MyPolicy from the interface

    // override interface policy to use FooPolicy instead
    @serializePolicy!FooPolicy
    Foo[] getFoos();

    @deserializePolicy!FooPolicy
    void setFoos(Foo[] foos);
}

I don't see much benefit in the @lean alone (I've created this PR mainly to reduce serialized data size when serializing empty fields in our REST API).
In normal serialization scenarios there are already de/serializeWithPolicy which can be used for the same purpose.
@s-ludwig what's your opinion?

@zunkree
Copy link

zunkree commented Apr 28, 2017

Any updates?

@tchaloupka
Copy link
Contributor Author

@s-ludwig we need a consensus here to move forward :)

@nazriel
Copy link
Contributor

nazriel commented Dec 11, 2017

@s-ludwig this is really desired feature ;)
I will probably patch my vibe-d with this - but I would love to see this in upstream.

so... formal BUMP! :)

@denizzzka
Copy link
Contributor

denizzzka commented Dec 18, 2018

Any news about this functionality?
Maybe it can be merged as an experimental? With appropriate messages.

@tchaloupka
Copy link
Contributor Author

rebased and fixed some errors in untested cases

WebFreak001 added a commit to WebFreak001/vibe.d that referenced this pull request Jan 8, 2020
Fixes vibe-d#1541 and is an alternative, simpler implementation of vibe-d#1650
WebFreak001 added a commit to WebFreak001/vibe.d that referenced this pull request Feb 19, 2020
Fixes vibe-d#1541 and is an alternative, simpler implementation of vibe-d#1650
@WebFreak001
Copy link
Contributor

the in/out argument for ignore is very useful, for optional it is imo pretty ambiguous.

I made a separate PR adding embedNullable (#2405) which got merged which was an alternative implementation of this functionality. For the optional part I think this PR is no longer needed, but for the ignore this could be useful.

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.

8 participants