Skip to content

Commit

Permalink
Repurpose the checkSchemaValidity to PrettyPrint only (#21987)
Browse files Browse the repository at this point in the history
--Remove the necessary elemnt check in im pretty print
--Rename CheckSchemaValidity to PrettyPrint
--Don't process the returned error for prettyPrint in IM client/server
code
  • Loading branch information
yunhanw-google authored Nov 17, 2022
1 parent 6221fe1 commit 751d0dd
Show file tree
Hide file tree
Showing 93 changed files with 732 additions and 1,641 deletions.
4 changes: 2 additions & 2 deletions config/esp32/components/chip/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ if (CONFIG_BUILD_CHIP_TESTS)
chip_gn_arg_bool("chip_build_tests" "true")
endif()

if (CONFIG_CHIP_ENABLE_SCHEMA_CHECK)
chip_gn_arg_bool("chip_enable_schema_check" "true")
if (CONFIG_IM_PRETTY_PRINT)
chip_gn_arg_bool("enable_im_pretty_print" "true")
endif()

if (NOT CONFIG_USE_MINIMAL_MDNS)
Expand Down
7 changes: 3 additions & 4 deletions config/esp32/components/chip/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -787,12 +787,11 @@ menu "CHIP Device Layer"
for the debug level events are disabled.


config CHIP_ENABLE_SCHEMA_CHECK
bool "Enable Schema Check"
config CHIP_CONFIG_IM_PRETTY_PRINT
bool "Enable IM Pretty Print"
default y
help
If enabled, it checks incoming messages are following the expected schema,
and incoming message payloads are logged in detail.
If enabled, incoming message payloads are logged in detail.
To see detailed logging please set default log level to Debug.
(Component config --> Log output --> Default log verbosity --> Debug)

Expand Down
40 changes: 20 additions & 20 deletions examples/common/tracing/decoder/interaction_model/Decoder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,12 @@ CHIP_ERROR LogAsProtocolMessage(uint8_t protocolCode, const uint8_t * data, size

CHIP_ERROR DecodeStatusResponse(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::StatusResponseMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
parser.PrettyPrint();
}
#endif

Expand All @@ -146,12 +146,12 @@ CHIP_ERROR DecodeStatusResponse(TLV::TLVReader & reader, bool decode)

CHIP_ERROR DecodeReadRequest(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::ReadRequestMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
return parser.PrettyPrint();
}
#endif

Expand All @@ -160,12 +160,12 @@ CHIP_ERROR DecodeReadRequest(TLV::TLVReader & reader, bool decode)

CHIP_ERROR DecodeSubscribeRequest(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::SubscribeRequestMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
return parser.PrettyPrint();
}
#endif

Expand All @@ -174,12 +174,12 @@ CHIP_ERROR DecodeSubscribeRequest(TLV::TLVReader & reader, bool decode)

CHIP_ERROR DecodeSubscribeResponse(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::SubscribeResponseMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
parser.PrettyPrint();
}
#endif

Expand All @@ -190,12 +190,12 @@ CHIP_ERROR DecodeReportData(TLV::TLVReader & reader, bool decode)
{
ReturnErrorOnFailure(MaybeDecodeNestedReadResponse(reader.GetReadPoint(), reader.GetTotalLength()));

#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::ReportDataMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
parser.PrettyPrint();
}
#endif

Expand All @@ -204,12 +204,12 @@ CHIP_ERROR DecodeReportData(TLV::TLVReader & reader, bool decode)

CHIP_ERROR DecodeWriteRequest(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::WriteRequestMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
return parser.PrettyPrint();
}
#endif

Expand All @@ -218,12 +218,12 @@ CHIP_ERROR DecodeWriteRequest(TLV::TLVReader & reader, bool decode)

CHIP_ERROR DecodeWriteResponse(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::WriteResponseMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
return parser.PrettyPrint();
}
#endif

Expand All @@ -234,12 +234,12 @@ CHIP_ERROR DecodeInvokeCommandRequest(TLV::TLVReader & reader, bool decode)
{
ReturnErrorOnFailure(MaybeDecodeNestedCommandRequest(reader.GetReadPoint(), reader.GetTotalLength()));

#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::InvokeRequestMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
return parser.PrettyPrint();
}
#endif

Expand All @@ -250,12 +250,12 @@ CHIP_ERROR DecodeInvokeCommandResponse(TLV::TLVReader & reader, bool decode)
{
ReturnErrorOnFailure(MaybeDecodeNestedCommandResponse(reader.GetReadPoint(), reader.GetTotalLength()));

#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::InvokeResponseMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
parser.PrettyPrint();
}
#endif

Expand All @@ -264,12 +264,12 @@ CHIP_ERROR DecodeInvokeCommandResponse(TLV::TLVReader & reader, bool decode)

CHIP_ERROR DecodeTimedRequest(TLV::TLVReader & reader, bool decode)
{
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#if CHIP_CONFIG_IM_PRETTY_PRINT
if (decode)
{
app::TimedRequestMessage::Parser parser;
ReturnErrorOnFailure(parser.Init(reader));
return parser.CheckSchemaValidity();
parser.PrettyPrint();
}
#endif

Expand Down
4 changes: 2 additions & 2 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import("common_flags.gni")

declare_args() {
# Enable strict schema checks.
chip_enable_schema_check =
enable_im_pretty_print =
is_debug && (current_os == "linux" || current_os == "mac" ||
current_os == "ios" || current_os == "android")

Expand All @@ -46,7 +46,7 @@ buildconfig_header("app_buildconfig") {
header_dir = "app"

defines = [
"CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK=${chip_enable_schema_check}",
"CHIP_CONFIG_IM_PRETTY_PRINT=${enable_im_pretty_print}",
"CHIP_CONFIG_IM_FORCE_FABRIC_QUOTA_CHECK=${chip_im_force_fabric_quota_check}",
"CHIP_CONFIG_ENABLE_SESSION_RESUMPTION=${chip_enable_session_resumption}",
"CHIP_CONFIG_ACCESS_CONTROL_POLICY_LOGGING_VERBOSITY=${chip_access_control_policy_logging_verbosity}",
Expand Down
4 changes: 2 additions & 2 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa
InvokeRequests::Parser invokeRequests;
reader.Init(std::move(payload));
VerifyOrReturnError(invokeRequestMessage.Init(reader) == CHIP_NO_ERROR, Status::InvalidAction);
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
VerifyOrReturnError(invokeRequestMessage.CheckSchemaValidity() == CHIP_NO_ERROR, Status::InvalidAction);
#if CHIP_CONFIG_IM_PRETTY_PRINT
invokeRequestMessage.PrettyPrint();
#endif

VerifyOrReturnError(invokeRequestMessage.GetSuppressResponse(&mSuppressResponse) == CHIP_NO_ERROR, Status::InvalidAction);
Expand Down
4 changes: 2 additions & 2 deletions src/app/CommandSender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ CHIP_ERROR CommandSender::ProcessInvokeResponse(System::PacketBufferHandle && pa
reader.Init(std::move(payload));
ReturnErrorOnFailure(invokeResponseMessage.Init(reader));

#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
ReturnErrorOnFailure(invokeResponseMessage.CheckSchemaValidity());
#if CHIP_CONFIG_IM_PRETTY_PRINT
invokeResponseMessage.PrettyPrint();
#endif

ReturnErrorOnFailure(invokeResponseMessage.GetSuppressResponse(&suppressResponse));
Expand Down
42 changes: 13 additions & 29 deletions src/app/MessageDef/AttributeDataIB.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,10 @@

namespace chip {
namespace app {
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
CHIP_ERROR AttributeDataIB::Parser::CheckSchemaValidity() const
#if CHIP_CONFIG_IM_PRETTY_PRINT
CHIP_ERROR AttributeDataIB::Parser::PrettyPrint() const
{
CHIP_ERROR err = CHIP_NO_ERROR;
int tagPresenceMask = 0;
CHIP_ERROR err = CHIP_NO_ERROR;
TLV::TLVReader reader;

PRETTY_PRINT("AttributeDataIB =");
Expand All @@ -51,11 +50,7 @@ CHIP_ERROR AttributeDataIB::Parser::CheckSchemaValidity() const
switch (tagNum)
{
case to_underlying(Tag::kDataVersion):
// check if this tag has appeared before
VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kDataVersion))), CHIP_ERROR_INVALID_TLV_TAG);
tagPresenceMask |= (1 << to_underlying(Tag::kDataVersion));
VerifyOrReturnError(TLV::kTLVType_UnsignedInteger == reader.GetType(), CHIP_ERROR_WRONG_TLV_TYPE);

#if CHIP_DETAIL_LOGGING
{
chip::DataVersion version;
Expand All @@ -64,24 +59,16 @@ CHIP_ERROR AttributeDataIB::Parser::CheckSchemaValidity() const
}
#endif // CHIP_DETAIL_LOGGING
break;
case to_underlying(Tag::kPath):
// check if this tag has appeared before
VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kPath))), CHIP_ERROR_INVALID_TLV_TAG);
tagPresenceMask |= (1 << to_underlying(Tag::kPath));
{
AttributePathIB::Parser path;
ReturnErrorOnFailure(path.Init(reader));
case to_underlying(Tag::kPath): {
AttributePathIB::Parser path;
ReturnErrorOnFailure(path.Init(reader));

PRETTY_PRINT_INCDEPTH();
ReturnErrorOnFailure(path.CheckSchemaValidity());
PRETTY_PRINT_DECDEPTH();
}
break;
PRETTY_PRINT_INCDEPTH();
ReturnErrorOnFailure(path.PrettyPrint());
PRETTY_PRINT_DECDEPTH();
}
break;
case to_underlying(Tag::kData):
// check if this tag has appeared before
VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kData))), CHIP_ERROR_INVALID_TLV_TAG);
tagPresenceMask |= (1 << to_underlying(Tag::kData));

PRETTY_PRINT_INCDEPTH();
ReturnErrorOnFailure(CheckIMPayload(reader, 0, "Data"));
PRETTY_PRINT_DECDEPTH();
Expand All @@ -97,15 +84,12 @@ CHIP_ERROR AttributeDataIB::Parser::CheckSchemaValidity() const
// if we have exhausted this container
if (CHIP_END_OF_TLV == err)
{
// check for required fields:
const int requiredFields = (1 << to_underlying(Tag::kPath)) | (1 << to_underlying(Tag::kData));

err = (tagPresenceMask & requiredFields) == requiredFields ? CHIP_NO_ERROR : CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_DATA_IB;
err = CHIP_NO_ERROR;
}
ReturnErrorOnFailure(err);
return reader.ExitContainer(mOuterContainerType);
}
#endif // CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#endif // CHIP_CONFIG_IM_PRETTY_PRINT

CHIP_ERROR AttributeDataIB::Parser::GetPath(AttributePathIB::Parser * const apPath) const
{
Expand Down
20 changes: 3 additions & 17 deletions src/app/MessageDef/AttributeDataIB.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,9 @@ enum class Tag : uint8_t
class Parser : public StructParser
{
public:
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
/**
* @brief Roughly verify the message is correctly formed
* 1) all mandatory tags are present
* 2) all elements have expected data type
* 3) any tag can only appear once
* 4) At the top level of the structure, unknown tags are ignored for forward compatibility
* @note The main use of this function is to print out what we're
* receiving during protocol development and debugging.
* The encoding rule has changed in IM encoding spec so this
* check is only "roughly" conformant now.
*
* @return #CHIP_NO_ERROR on success
*/
CHIP_ERROR CheckSchemaValidity() const;
#endif

#if CHIP_CONFIG_IM_PRETTY_PRINT
CHIP_ERROR PrettyPrint() const;
#endif // CHIP_CONFIG_IM_PRETTY_PRINT
/**
* @brief Get the DataVersion.
*
Expand Down
8 changes: 4 additions & 4 deletions src/app/MessageDef/AttributeDataIBs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ using namespace chip::TLV;

namespace chip {
namespace app {
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
CHIP_ERROR AttributeDataIBs::Parser::CheckSchemaValidity() const
#if CHIP_CONFIG_IM_PRETTY_PRINT
CHIP_ERROR AttributeDataIBs::Parser::PrettyPrint() const
{
CHIP_ERROR err = CHIP_NO_ERROR;
size_t numDataElement = 0;
Expand All @@ -59,7 +59,7 @@ CHIP_ERROR AttributeDataIBs::Parser::CheckSchemaValidity() const
ReturnErrorOnFailure(data.Init(reader));

PRETTY_PRINT_INCDEPTH();
ReturnErrorOnFailure(data.CheckSchemaValidity());
ReturnErrorOnFailure(data.PrettyPrint());
PRETTY_PRINT_DECDEPTH();
}

Expand All @@ -81,7 +81,7 @@ CHIP_ERROR AttributeDataIBs::Parser::CheckSchemaValidity() const
ReturnErrorOnFailure(err);
return reader.ExitContainer(mOuterContainerType);
}
#endif // CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
#endif // CHIP_CONFIG_IM_PRETTY_PRINT

AttributeDataIB::Builder & AttributeDataIBs::Builder::CreateAttributeDataIBBuilder()
{
Expand Down
19 changes: 3 additions & 16 deletions src/app/MessageDef/AttributeDataIBs.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,22 +40,9 @@ namespace AttributeDataIBs {
class Parser : public ArrayParser
{
public:
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
/**
* @brief Roughly verify the message is correctly formed
* 1) all mandatory tags are present
* 2) all elements have expected data type
* 3) any tag can only appear once
* 4) At the top level of the structure, unknown tags are ignored for forward compatibility
* @note The main use of this function is to print out what we're
* receiving during protocol development and debugging.
* The encoding rule has changed in IM encoding spec so this
* check is only "roughly" conformant now.
*
* @return #CHIP_NO_ERROR on success
*/
CHIP_ERROR CheckSchemaValidity() const;
#endif
#if CHIP_CONFIG_IM_PRETTY_PRINT
CHIP_ERROR PrettyPrint() const;
#endif // CHIP_CONFIG_IM_PRETTY_PRINT
};

class Builder : public ArrayBuilder
Expand Down
Loading

0 comments on commit 751d0dd

Please sign in to comment.