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

Detecting "Invalid" Parsing of {"test":} and {"test":unquoted_string} (v5) #842

Open
jeffsf opened this Issue Nov 5, 2018 · 6 comments

Comments

2 participants
@jeffsf

jeffsf commented Nov 5, 2018

First, many thanks for the hours of work that have gone into this useful library. Despite the well-chosen name, it appears that it will run without the Arduino framework given the proper header/library support. Getting off the Arduino framework is on my short list for a long list of reasons.

For context, parsing for my application needs to look for a specific "command" key, then deal with the associated value. That value can be "any" valid JSON value. In particular, for a given command, null, "all", "0x41", 65, a JSON object, or a JSON array all are "valid".

I can see some possible reasons that the JSON key is successfully parsed without quotes, given my perception of Arduino's target market. Past hoping that v6 includes a "strict" mode is available, the key I can deal with as it can be transparently compared to the "valid" command strings. (Might be "really strict" mode, given #44)

Where I run into challenges is detecting the empty-value "degenerate" case {"test":} or the unquoted-string case {"test":unquoted_string} cleanly and reliably.

For the empty-value case, parsed["test"].measureLength() == 0 seems to handle it, but relies on undocumented behavior. I poked around a bit looking for a way to robustly detect the "null-safe" JsonVariant object, but wasn't successful. (It looks like JsonVariant::isNull() in v6 might address this for the future.)

For the unquoted-string case, it seems to come down to the use of a default case of "well, it isn't a documented type, and it isn't zero-length." Were another type to be added, this approach would fail.

Is there a better way to accomplish this?

Test code below compiled with ArduinoJson 5.13.3, platformio on macOS, ATmega328P (uno) target.

Most "succeed" in properly identifying the value's type using properly sequenced .is<T>() tests. Notable exceptions are:

{"test":}
zero-length, degenerate case that shouldn't have parsed

{"test":unquoted_string}
No idea how it got here

(and their {test: counterparts)

#include <Arduino.h>

#include <avr/pgmspace.h>

#include <stddef.h>

#include "ArduinoJson.h"

#include "pf.h"

void
setup()
{
    Serial.begin(115200);
    while ( ! Serial ) continue;

    const char* test_strings[]
            {

        R"({})",

        // NOT quoted -- test:

        R"({test:})",
        R"({test:null})",
        R"({test:true})",
        R"({test:false})",
        R"({test:5})",
        R"({test:5.5})",
        R"({test:{}})",
        R"({test:[]})",

        R"({test:""})",
        R"({test:"null"})",
        R"({test:"true"})",
        R"({test:"false"})",
        R"({test:"5"})",
        R"({test:"5.5"})",
        R"({test:"{}"})",
        R"({test:unquoted_string})",

        // quoted -- "test":

        R"({"test":})",
        R"({"test":null})",
        R"({"test":true})",
        R"({"test":false})",
        R"({"test":5})",
        R"({"test":5.5})",
        R"({"test":{}})",
        R"({"test":[]})",

        R"({"test":""})",
        R"({"test":"null"})",
        R"({"test":"true"})",
        R"({"test":"false"})",
        R"({"test":"5"})",
        R"({"test":"5.5"})",
        R"({"test":"{}"})",
        R"({"test":unquoted_string})",

            };

    for ( const char* str : test_strings) {
        Serial.println(str);

        StaticJsonBuffer<300> jsonBuffer;

        JsonObject &parsed = jsonBuffer.parse(str);

        if ( !parsed.success()) {
            Serial.println("FAIL");

        } else {
            if ( false ) {
                // just so tests can always start with } else if

            // does not catch {"test":}
            } else if ( not parsed["test"].success()) {
                Serial.print(F("no success(), nothing to see, move along\n"));

//        // size() isn't helpful in determining if a token is present or not
//        } else if ( ! parsed["test"].size() ) {
//            Serial.print(F("no size(), nothing to see, move along\n"));

            } else if ( parsed["test"].is<char *>()) {
                // could be JSON null here
                auto value_as_char_p = parsed["test"].as<char *>();
                if ( value_as_char_p ) {
                    pf_P(PSTR("char*: |%s| of length %d\n"), value_as_char_p, strlen(value_as_char_p));
                } else {
                    Serial.print(F("null\n"));
                }

            } else if ( parsed["test"].is<int>()) {
                auto value_as_int = parsed["test"].as<int>();
                pf_P(PSTR("int: %d\n"), value_as_int);

            // check float after int, as int will populate float
            } else if ( parsed["test"].is<float>()) {
                auto value_as_float = parsed["test"].as<float>();
                Serial.print(F("float: "));
                Serial.print(value_as_float);
                Serial.print('\n');

            } else if ( parsed["test"].is<bool>()) {
                auto value_as_bool = parsed["test"].as<bool>();
                pf_P(PSTR("bool: %d\n"), value_as_bool);

            } else if ( parsed["test"].is<JsonObject>()) {
                Serial.print(F("object\n"));

            } else if ( parsed["test"].is<JsonArray>()) {
                Serial.print(F("array\n"));

            } else if ( parsed["test"].measureLength() == 0 ) {
                Serial.print(F("zero-length, degenerate case that shouldn't have parsed\n"));

            } else {
                Serial.print(F("No idea how it got here\n"));

            }
        }
        Serial.print('\n');
    }

}

void
loop()
{}

pf.* abridged, in the interest of space

#include <stdarg.h>
#include <stdio.h>

#include <Arduino.h>

#define PFMAX 100

void
pf_P(const char* format, ...)
{
    char buffer[PFMAX];
    va_list args;
    va_start(args, format);
    vsnprintf_P(buffer, PFMAX, format, args);
    va_end(args);
    Serial.print(buffer);
}
@bblanchon

This comment has been minimized.

Owner

bblanchon commented Nov 5, 2018

Hi @jeffsf,

I don't think there is a better way to do that with version 5, because it was designed to be very lenient.

ArduinoJson 6, however, is more strict. Since version 6.2.0, values must be quoted (but not keys).
Here is a demo with your test strings: https://wandbox.org/permlink/8kPGuIX8KJnMqUBX)
I think it does exactly what you want.

Regards,
Benoit

@bblanchon bblanchon added the question label Nov 5, 2018

@jeffsf

This comment has been minimized.

jeffsf commented Nov 6, 2018

Greatly appreciated. Thank you for the quick response!

I'll take a look at v6 and see how it might simplify my overall code. I'm assuming that the run-time RAM utilization is comparable to v5, yes?

BTW, Payhip isn't cooperating with PayPal for your book -- "Things don't appear to be working at the moment. Please try again later." with {Return to Merchant] as being the only option.

@bblanchon

This comment has been minimized.

Owner

bblanchon commented Nov 6, 2018

Hi @jeffsf,

The RAM utilization in v6 is comparable to v5, just slightly higher.
I'm upgrading the internal structure to improve the performance, and I'm upgrading the allocator so it can release memory as most users expect. That's why the code is a little bigger, and the RAM usage is a little higher.

I'm not aware of any problem on PayHip, but I'd be very interested in knowing what happened.
Can you try again? If it doesn't work, can you try from this page: https://payhip.com/b/1lHw

The book currently covers ArduinoJson 5.13.
I'll publish a new revision when v6 is out of beta, in early 2019.

Regards,
Benoit.

@jeffsf

This comment has been minimized.

jeffsf commented Nov 6, 2018

6.x branch in git working here in CLion under macOS High Sierra and XCode toolchain. On HEAD right now, and will watch commits as I go.

PayHip worked this morning. Last night it was showing the PayPal form briefly on the pop-up after PayHip collect info, click pay process, then replacing it almost immediately with the "Things don't appear to be working..." content.

That I can work through use of ArduinoJson without ever touching a MCU is a huge plus! Kudos on what must have been a non-trivial task! Perhaps worth highlighting this approach in your documentation for developers (with appropriate notes about changing platforms will change memory requirements for the same input).

Edit: I'm looking into MessagePack now as an alternative to using structs for persisting configuration in EEPROM. While structs great for compactness and aligning with 64-byte erase sectors, I'm dreading carrying along the baggage of every previous version when the application evolves over time. That it encodes binary data and floats in a reasonable way makes handling things like device I2C configuration registers and calibration parameters not much less dense than a struct.

Working for me in CLion:

cmake_minimum_required(VERSION 3.12)
project(main)

set(CMAKE_CXX_STANDARD 14)

include_directories(
        lib/ArduinoJson/src
        )

add_executable(main
        src/main.cpp
        )
@bblanchon

This comment has been minimized.

Owner

bblanchon commented Nov 7, 2018

Hi @jeffsf,

ArduinoJson is not able to use EEPROM directly.
Maybe that's something that I can add in the future.

For now, ArduinoJson doesn't support the "binary family" of MessagePack.
I'll add it later.

Regards,
Benoit

@jeffsf

This comment has been minimized.

jeffsf commented Nov 7, 2018

No problem on not working with EEPROM directly. Between on-chip EEPROM, chip-emulated EEPROM, SPI EEPROM, and who knows what else, that's quite a mess. As long as I can get a buffer with the bytes in it, I can figure out how to get it into EEPROM and back out again.

Good to know on the status of "binary" data at present. Dealing with "upgraded" is definitely a forward-looking thing that I've kicked down the road, save for a version field at the start of the current struct.

Definitely appreciate the ease of use of the v6 API compared to v5!

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