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

Problem with optional fields is not solved #6

Open
uralm1 opened this issue Nov 14, 2024 · 6 comments
Open

Problem with optional fields is not solved #6

uralm1 opened this issue Nov 14, 2024 · 6 comments

Comments

@uralm1
Copy link

uralm1 commented Nov 14, 2024

As I see, your fork doesn't solve the problem with optional fields in api structures and json.

Let's see.

sendMessage(... LinkPreviewOptions::Ptr linkPreviewOptions = nullptr ...)

Ok, we can remove LinkPreviewOptions structure from output json COMPLETELY when it null.

class LinkPreviewOptions {
  
  public:
    ⦙ typedef std::shared_ptr<LinkPreviewOptions> Ptr;
  
    ⦙ /**
    ⦙ ⦙* @brief Optional. True, if the link preview is disabled
    ⦙ ⦙*/
    ⦙ bool isDisabled;
  
    ⦙ /**
    ⦙ ⦙* @brief Optional. URL to use for the link preview.
    ⦙ ⦙*
    ⦙ ⦙* If empty, then the first URL found in the message text will be used
    ⦙ ⦙*/
    ⦙ std::string url;
  
    ⦙ /**
    ⦙ ⦙* @brief Optional. True, if the media in the link preview is supposed to be shrunk; ignored if the URL is
    ⦙ ⦙*/
    ⦙ bool preferSmallMedia;

We cannot include only a PART of LinkPreviewOptions structure in output json. Thats why I suggest wrapping fields in boost::optional.
It's possible to check optionality from pointer field, but we cannot make optional bools, ints or strings (ok, strings - disputable).

class ReplyParameters {
  
  public:
    ⦙ typedef std::shared_ptr<ReplyParameters> Ptr;
  
    ⦙ /**
    ⦙ ⦙* @brief Identifier of the message that will be replied to in the current chat, or in the chat chatId if 
    ⦙ ⦙*/
    ⦙ std::int32_t messageId;
  
    ⦙ /**
    ⦙ ⦙* @brief Optional. If the message to be replied to is from a different chat, unique identifier for the ch
    ⦙ ⦙*
    ⦙ ⦙* Not supported for messages sent on behalf of a business account.
    ⦙ ⦙*/
    ⦙ std::int64_t chatId;

Need to skip chatId.

DECLARE_PARSER_TO_JSON(ReplyParameters) {
    ⦙ JsonWrapper json;
    
    ⦙ if (object) {
    ⦙ ⦙ ⦙ json.put("message_id", object->messageId);
    ⦙ ⦙ ⦙ json.put("chat_id", object->chatId);

ChatId is always serialized as 0 if it was not assigned implicitly.

@uralm1
Copy link
Author

uralm1 commented Nov 14, 2024

Addition: I looked in metaprograming code in TgTypeParser.h, I think this serializers don't help the issue. Am I wrong?

is_initialized_with_default_constructor_v
is_assigned_by_hand traits can help :)

@uralm1
Copy link
Author

uralm1 commented Nov 16, 2024

I've made some research, how resulting json is generated, and found that json.put() uses heuristic hacks depended of value type to determine, include field to resulting json or not. It can generate incorrect results sometimes:
For example:

ReplyParameters.messageId = 0 generates json without messageId key, but messageId is NOT optional -> wrong,
ReplyParameters.chatId = 0 generates json without chatId key, chatId is optional -> right,
ReplyParameters.quote_entities - always generated as [] when is not set -> wrong
etc

Lack of possibility to include false for bool value, 0 for int value to resulting json is not good.

@Royna2544
Copy link
Owner

Royna2544 commented Nov 18, 2024

always generated as [] when is not set -> wrong

I guess this is easily fixable

0 generates json without messageId key, but messageId is NOT optional -> wrong,

For this, I'll probably have to think how-to
(Probably specializing JsonWrapper::put for std::optional, and wrapping the optional object all in std optional would fix it)

Royna2544 added a commit that referenced this issue Nov 18, 2024
@Royna2544
Copy link
Owner

It should be fixed now as I wrapped really optional objects under std optional, can you try it?

@uralm1
Copy link
Author

uralm1 commented Nov 18, 2024

I'm experimenting with optional attributes here reo7sp#320, keep an eye there too.

Also wanted to add OptionalEmptyString<>, OptionalFalseBool<> attributes, but don't know how to differentiate it from Required types.

@Royna2544
Copy link
Owner

Royna2544 commented Nov 18, 2024

Meh, the one without jsoncpp is a mess, as i said before, doesn't work (gives a bad request) for createStickerSet or setBotCommands(Windows) etc, (Why even manually concat json string when there is jsoncpp or even nlohmann/json which is header only?) So I'm not interested.

OptionalEmptyString<>, OptionalFalseBool<>

And I think they should be wrapped in a empty struct tag, so they can be figured out with metaprogramming.

My updated code with std::optional doesn't enforce the required parameters to be set, but it will indeed put the required args in JSON now, so I think mine is the best solution.

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

No branches or pull requests

2 participants