JSON import/export: added base64 support for [ubyte] array and nested FBS - #4585

I can't confirm by reading the code, but I added the test case for input and wrote the expected output in padded strict JSON. This passes the flatcc json test suite including verifying the nested buffer (which is kind of lucky since the monster_test.fbs are not exactly identical).

dvidelabs/flatcc@9ed2733

Note that the test fails if flatcc compiles with UQ off (unquoted), meaning the test case fails when it expects strict json, but that is to be expected.

I have released my implementation in v0.5.0 earlier today so the above is not in the release.

Note that I did not add runtime flags to control printing with or without padding - mostly because it was messy in the input, as you said. But it could be added later. I think what we have is fine for interop.

BTW: the TEST macro in my test takes two string arguments, the first is the input of a monster_test.fbs json file and the second argument is the expected json printed output without spaces if the parse is successful.

I tried to test the schema, but some semantics have diverged a bit on namespaces and language specific attributes. Removing these from the schema and the schema parses cleanly with base64, base64url attributes.

At commit 79b80f8 the method ReserveElements was removed from FlatBufferBuilder class.
ReserveElements
My code uses this public method for memory preallocation.
How to preallocate a big Vector without using temporary objects like std::vector<>?

@aardappel reservation in some form is necessary, otherwise the base64 decoder becomes very complex. The output size can be estimated conservatively within a small margin allowing the decoder to run without having to check output boundaries and manage flushing.

Resolved.
Problem with ReserveElements can be solved with method:
uoffset_t CreateUninitializedVector(size_t len, size_t elemsize, uint8_t **buf)

I just realized that this also needs to be tested with union strings and string vectors, and union vectors with strings.

Can you present examples of these cases?
I'm finishing requested changes within a few days.
At the first stage (this PR), only [ubyte] arrays are supported.
At the second, nested buffers.

I got confused - I forgot it was only ubyte and [ubyte] cannot be part of unions.

I was thinking of a use case like loading multiple PEM certificates where each cert type would have different union type so the following could be handled

http://how2ssl.com/articles/working_with_pem_files/

This would currently require each cert ot be loaded into a separate table, but may that is not a bad thing.

I\m not sure exactly what the latest changes do, but it appears to enforcing padding.

flatcc isn't very configurable right now, but I believe it accepts unpadded input and in the future will be allowed to generate unpadded.

There are some JSON base64 formats floating around that define base64 to be unpadded, so I don't think it is wise to reject it, given that the string is fully delimited.

This is protocol buffers base64: "JSON value will be the data encoded as a string using standard base64 encoding with paddings. Either standard or URL-safe base64 encoding with/without paddings are accepted."
https://developers.google.com/protocol-buffers/docs/proto3#json

@mikkelfj,
Thank you for review and you are right.
This commit with a default base64 decode/encode behaviour was not good. This was experiment with code. As we discussed earlier encoder/decoder behavior must satisfy:
base64/base64url decoder: padding is optional,
base64 output: with padding,
base64url output: with padding and have IDL-option JsonBase64CancelPadding.

@vglavnyy note there is one new suggestion: that base64 decodes also base64url and that base64url also decodes base64 unless explicitly required not to by some optional configuration

Currently I'm not sure if flatcc does this, but I know that the implementation is prepared for it and I'd be happy to make this update later.

The (base64url) decoder implicitly can decode sequence form (base64) encoder, but versa does not.
I can add compatibility (base64url)=>(base64).
But this will be further away from the RFC specification.
Even (base64)=>(base64url) compatibility violates RFC.

@aardappel ,
I done almost all requested changes and did rebase to master.
Now I have two unresolved problem.
First:
To the file "util_base64.h" aded templated function field_base64_mode.
It used in two places now. But if add nested buffers support it will be used in additional two places.
Should I delete this function or move it to another file?

Second:

Change requets for implementation of PrintVectorBase64 function:
why does this function encode into a vector and not a string? If it took a string, we could pass it text, and it could append straight where it is needed. See similarly how EscapeString is called.

PrintVectorBase64

Use appeding of characters to a string (like EscapeString) is impossible for current implementation of base64 encoder. Base64 encoder use direct pointer for writing to destination memory.
The vector copy can be avoided if use direct write to the string internal memory after extending it append(0, req_len). Address of memory can be obtained using &test[x] operator.
C++11 gives guarantee that internal memory of a std::string use a contiguous block of memory. But I'm not sure with correctness of this solution.

@aardappel , could you review latest changes in the base-PR code?

Any ETA on when this PR is finalized and merged? I'd like to use it for some projects of mine (among which, https://github.com/KageKirin/flatGLTF)

@aardappel, Thank you for your review.
You are completely right about the implementation of the PrintVectorBase64 function and I fixed it.
Can we split review of this PR into two parts?

First part is an interface between the Flatbuffers and a base64 routines. The current interface declared in the “base64_utils.h” and used in the "idl_gen_text.cpp", "idl_parse.cpp" and "test.cpp". The enum “Base64Mode” define working modes for decode/encode routines.
The encoder has a simple interface. The decoder has two routines: Base64DecodedSize and Base64Decode. The current implementation of the decoder can return the position of a first invalid symbol inside a base64 string. This restricts a possible implementation of a base64 decoder. It can be removed from the interface and from the tests.

The second part is an implementation of base64 routines. There are a lot of implementations of base64 encode/decode routines and anyone can find or write own.
The proposed implementation located in the base64_utils.cpp. But for the first commit, it can be replaced by stub which does not violate current behavior of the Flatbuffers.

I'm not sure what all this splitting up buys us. My exact problem with the current PR is that there's an mismatch: the core routines have C-style interfaces that require copying by the user. I proposed the opposite: that the core routines directly work with the types already used by the parser/generator, such that there is less translation steps. So if anything, less splitting up, more integration.

Ok. I understand your proposal.
And I propose closing this PR and return to the issue stage.
We can take the already defined attributes "base64" and "base64url" as a basis for the issue (with or without the IDL option "base64_cancel_padding").
Then once again discussing the behavior of the encoder and decoder for these attributes if that behavior is not clear. After that, someone can propose a new solution for this issue.

I don't think I understand. Why not just fix this PR with my suggestions?

I don't know how to implement your suggestions and I need advice.
Let's start from simple case of printing.

Current implementation

template<typename T>
bool PrintVector(const Vector<T> &v, Type type, int indent,
                 const IDLOptions &opts, std::string *_text,
                 const FieldDef *fd = nullptr) {
  std::string &text = *_text;

  // Try to print UCHAR array as base64 string.
  if (type.base_type == BASE_TYPE_UCHAR) {
    auto b64mode = FieldGetBase64Mode(fd);
    if (b64mode) {
      text += "\"";
      Base64Encode(
          opts.base64_cancel_padding ? Base64CancelPadding(b64mode) : b64mode,
          reinterpret_cast<const uint8_t *>(v.data()), v.size(), &text);
      text += "\"";
      return true;
    }
  }
  .....
}

Proposed:

// Interface declaration from future version of the util_base64.h
// Try to print the vector of UCHAR as base64 string.
// Returns false if the field fd doesn't have base64 attribute.
bool PrintBase64Vector(const Vector<UCHAR> &v,  // data source
                       Type type, int indent, const IDLOptions &opts,
                       std::string *_text,  // destination
                       const FieldDef *fd);

Future implementation of PrintVector from idl_gen_text.cpp

template<typename T>
bool PrintVector(const Vector<T> &v, Type type, int indent,
                 const IDLOptions &opts, std::string *_text,
                 const FieldDef *fd = nullptr) {
  std::string &text = *_text;
  if (type.base_type == BASE_TYPE_UCHAR) {
    // very ugly reinterpret cast
    PrintBase64Vector(*(reinterpret_cast<const Vector<UCHAR> *>(&v)), type,
                      ident, opts, _text, fd)
  }
  ....
  ....
}

How to add other types not only UCHAR?
Templated version of the PrintBase64Vector will break encapsulation and will lead to the current version of PrintVector.

Yes, you'll likely need templates. Break what encapsulation? There's no need for encapsulation of anything here.

@aardappel, could you review this PR again?
I rewrote interface to the base64 routines and removed low-level core routines from the header file.
Is this the correct way?

I apologize for wasting your time.
This problem is harder for me than I thought.
I hope this issue will be solved by someone with more acceptable way.
If a one needs a temporary solution, I have made it as the addon with source code under CC0.

Why don't you simply address my code review suggestions? From your other code you are clearly capable of doing so. The biggest waste of time is creating a feature and then not merging it because you don't want to make changes to your code.

The problem was with me and my hidden goal.
My project requires high-performance base64 encoder and decoder with AVX2 and NEON optimizations.
Suggested approach with the symbol by symbol appending to the string will block the optimization.

Could we once again return to the beginning of PR?
Here is the updated proposal of the base64 interface base64-PR2.
This interface has only two unimplemented routines:

void util_base64_encode(int b64mode, const uint8_t *src, size_t src_size, std::string *_text);
size_t util_base64_decode(int b64mode, const std::string &src, uint8_t *dst, size_t dst_size);

I believe that these routines should be implemented inside util_base64.cpp without dependencies from flatbuffers. In this case, one can easily replace the encoder/decoder if it is needed.
Is this interface acceptable?

So, I know Wouters position is different here. But in my view, the "reserve" facility in STL i exactly designed for the case where you feed in a known quantity of data or need to process that data in-memory. So while I can that in most cases messing with reserve is just code obfuscation, I think that does not apply here. Especially considering that a base64 string could be in the size of many megabytes.

Now I agree with Wouter. The code should be simple and clean and optimization doesn't help to make the code simple.
If util_base64_encode and util_base64_decode are pure functions this will keep simplicity and flexibility.

In the proposed solution, only CreateUninitializedVector can't be avoided.
Memory for decoded array should be reserved before decode.
Calculation of required size might be very simple as R=text.size()*3/4 if to trim preallocated vector to written size after decoding.
This "*3/4" guess will differ from the real written number no more than 2 bytes. It depends on trailing padding in the source base64 string.

Update:
I rewrote the ParseBase64Vector: base64-PR2.

All my suggestions are in my past code-review comments.