Skip to content

feat: Augment schema parser error messages #277

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.

Sign up for GitHub

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

Open
tbkr wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

feat: Augment schema parser error messages #277

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6399d0d
feat: Augment schema parser error messages
tbkr May 11, 2026
d144eea
feat: Error for non-ascii character in schema
tbkr Jun 1, 2026
6e4d27a
refactor: Review remarks
tbkr Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/fdb5/rules/Schema.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "eckit/exception/Exceptions.h"
#include "eckit/filesystem/PathName.h"
#include "eckit/log/Log.h"
#include "eckit/parser/StreamParser.h"
#include "eckit/utils/Tokenizer.h"

#include "fdb5/LibFdb5.h"
Expand Down Expand Up @@ -197,7 +198,14 @@ void Schema::load(const eckit::PathName& path, const bool replace) {
throw ex;
}

load(in, replace);
try {
load(in, replace);
}
catch (SchemaParser::Error& spe) {

@simondsmart simondsmart Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand the point of this try/catch wrapper. It catches a SchemaParser - and throws a SchemaParser. There is no extra information to add here?

@tbkr tbkr Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Enriching the context of the error message by telling what the actual underlying issue is.

std::stringstream buf;
buf << "Error loading FDB schema file: " << path << ". Underlying issue: " << spe.what();
throw SchemaParser::Error(buf.str());
}
}

void Schema::load(std::istream& s, const bool replace) {
Expand Down
153 changes: 104 additions & 49 deletions src/fdb5/rules/SchemaParser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
/// @date April 2016

#include "fdb5/rules/SchemaParser.h"
#include "eckit/parser/StreamParser.h"
#include "fdb5/rules/ExcludeAll.h"
#include "fdb5/rules/MatchAlways.h"
#include "fdb5/rules/MatchAny.h"
Expand All @@ -24,12 +25,30 @@
#include "fdb5/rules/Rule.h"

#include <memory>
#include <set>
#include <sstream>
#include <utility>
Comment thread
tbkr marked this conversation as resolved.

namespace fdb5 {

//----------------------------------------------------------------------------------------------------------------------

bool SchemaParser::isAscii(char c) {
return static_cast<unsigned char>(c) < 128;
}

char SchemaParser::peek(bool spaces) {
const char peeked = parser.peek(spaces);

if (peeked != 0 && !isAscii(peeked)) {
std::stringstream buf;
buf << "Schema file contained non-ASCII character which are not supported." << std::endl;
throw eckit::StreamParser::Error(buf.str(), parser.line() + 1);
}

return peeked;
}
Comment thread
tbkr marked this conversation as resolved.

std::string SchemaParser::parseIdent(bool value, bool emptyOK) {
std::string s;
for (;;) {
Expand All @@ -45,19 +64,19 @@ std::string SchemaParser::parseIdent(bool value, bool emptyOK) {
case ']':
case '?':
if (s.empty() && !emptyOK) {
throw StreamParser::Error("Syntax error: found '" + std::to_string(c) + "'", line_ + 1);
throw eckit::StreamParser::Error("Syntax error: found '" + std::string{c} + "'", parser.line() + 1);
}
Comment thread
tbkr marked this conversation as resolved.
return s;
case '-':
if (s.empty() && !emptyOK) {
throw StreamParser::Error("Syntax error: found '-'", line_ + 1);
throw eckit::StreamParser::Error("Syntax error: found '-'", parser.line() + 1);
}
if (!value) {
return s;
}
[[fallthrough]];
default:
consume(c);
parser.consume(c);
s += c;
break;
}
Expand All @@ -73,19 +92,19 @@ std::unique_ptr<Predicate> SchemaParser::parsePredicate(eckit::StringDict& types
char c = peek();

if (c == ':') {
consume(c);
parser.consume(c);
ASSERT(types.find(k) == types.end());
types[k] = parseIdent(false, false);
c = peek();
}

if (c == '?') {
consume(c);
parser.consume(c);
return std::make_unique<Predicate>(k, new MatchOptional(parseIdent(true, true)));
}

if (c == '-') {
consume(c);
parser.consume(c);
if (types.find(k) == types.end()) {
// Register ignore type
types[k] = "Ignore";
Expand All @@ -94,7 +113,7 @@ std::unique_ptr<Predicate> SchemaParser::parsePredicate(eckit::StringDict& types
}

if (c != ',' && c != '[' && c != ']') {
consume("=");
parser.consume("=");

std::string val = parseIdent(true, false);
exclude = val[0] == '!';
Expand All @@ -107,7 +126,7 @@ std::unique_ptr<Predicate> SchemaParser::parsePredicate(eckit::StringDict& types
}

while ((c = peek()) == '/') {
consume(c);
parser.consume(c);
values.insert(parseIdent(true, false));
}
}
Expand All @@ -126,30 +145,41 @@ std::unique_ptr<Predicate> SchemaParser::parsePredicate(eckit::StringDict& types
}

void SchemaParser::parseTypes(eckit::StringDict& types) {
for (;;) {
const auto name = parseIdent(false, true);
if (name.empty()) {
break;

try {
for (;;) {
const auto name = parseIdent(false, true);
if (name.empty()) {
break;
}
parser.consume(':');
const auto type = parseIdent(false, false);
parser.consume(';');
ASSERT(types.find(name) == types.end());
types[name] = type;
}
consume(':');
const auto type = parseIdent(false, false);
consume(';');
ASSERT(types.find(name) == types.end());
types[name] = type;
}
catch (eckit::StreamParser::Error& spe) {
std::stringstream buf;
buf << "SchemaParser::parseTypes: Error during parsing of types in schema, check the definitions: '<name>: "
"<type>;'."
<< " Underlying issue: " << spe.what();

throw Error(buf.str(), parser.line() + 1);
}
Comment thread
tbkr marked this conversation as resolved.
}

std::unique_ptr<RuleDatum> SchemaParser::parseDatum() {
Rule::Predicates predicates;
eckit::StringDict types;

consume('[');
parser.consume('[');

const std::size_t line = line_ + 1;
const std::size_t line = parser.line() + 1;

char c = peek();
if (c == ']') {
consume(c);
parser.consume(c);
return std::make_unique<RuleDatum>(line, predicates, types);
}

Expand All @@ -159,13 +189,13 @@ std::unique_ptr<RuleDatum> SchemaParser::parseDatum() {

predicates.emplace_back(parsePredicate(types));
while ((c = peek()) == ',') {
consume(c);
parser.consume(c);
predicates.emplace_back(parsePredicate(types));
}

c = peek();
if (c == ']') {
consume(c);
parser.consume(c);
return std::make_unique<RuleDatum>(line, predicates, types);
}
}
Expand All @@ -176,13 +206,13 @@ std::unique_ptr<RuleIndex> SchemaParser::parseIndex() {
eckit::StringDict types;
RuleIndex::Child rule;

consume('[');
parser.consume('[');

const std::size_t line = line_ + 1;
const std::size_t line = parser.line() + 1;

char c = peek();
if (c == ']') {
consume(c);
parser.consume(c);
return std::make_unique<RuleIndex>(line, predicates, types, std::move(rule));
}

Expand All @@ -196,14 +226,14 @@ std::unique_ptr<RuleIndex> SchemaParser::parseIndex() {
else {
predicates.emplace_back(parsePredicate(types));
while ((c = peek()) == ',') {
consume(c);
parser.consume(c);
predicates.emplace_back(parsePredicate(types));
}
}

c = peek();
if (c == ']') {
consume(c);
parser.consume(c);
return std::make_unique<RuleIndex>(line, predicates, types, std::move(rule));
}
}
Expand All @@ -214,37 +244,47 @@ std::unique_ptr<RuleDatabase> SchemaParser::parseDatabase() {
eckit::StringDict types;
RuleDatabase::Children rules;

consume('[');
try {
parser.consume('[');

const std::size_t line = line_ + 1;
const std::size_t line = parser.line() + 1;

char c = peek();
if (c == ']') {
consume(c);
return std::make_unique<RuleDatabase>(line, predicates, types, rules);
}
char c = peek();
if (c == ']') {
parser.consume(c);
return std::make_unique<RuleDatabase>(line, predicates, types, rules);
}

for (;;) {
for (;;) {

c = peek();
c = peek();

if (c == '[') {
rules.emplace_back(parseIndex());
}
else {
predicates.emplace_back(parsePredicate(types));
while ((c = peek()) == ',') {
consume(c);
if (c == '[') {
rules.emplace_back(parseIndex());
}
else {
predicates.emplace_back(parsePredicate(types));
while ((c = peek()) == ',') {
parser.consume(c);
predicates.emplace_back(parsePredicate(types));
}
}
}

c = peek();
if (c == ']') {
consume(c);
return std::make_unique<RuleDatabase>(line, predicates, types, rules);
c = peek();
if (c == ']') {
parser.consume(c);
return std::make_unique<RuleDatabase>(line, predicates, types, rules);
}
}
}
catch (eckit::StreamParser::Error& spe) {
std::stringstream buf;
buf << "SchemaParser::parseDatabase: Error during parsing of rules in schema, check for closing brackets and "
"definitions."
<< " Underlying issue: " << spe.what();

throw Error(buf.str(), parser.line() + 1);
}
Comment thread
tbkr marked this conversation as resolved.
}

void SchemaParser::parse(RuleList& result, TypesRegistry& registry) {
Expand All @@ -261,10 +301,25 @@ void SchemaParser::parse(RuleList& result, TypesRegistry& registry) {
}

if (c) {
throw StreamParser::Error(std::string("Error parsing rules: remaining char: ") + c);
throw Error(std::string("SchemaParser::parse: Error parsing rules: remaining char: ") + c);
}


if (result.size() == 0) {
std::stringstream buf;
buf << "SchemaParser::parse: Empty rule list. Didn't find any rule in the provided schema file." << std::endl;
throw Error(buf.str());
}
Comment thread
tbkr marked this conversation as resolved.
}

//----------------------------------------------------------------------------------------------------------------------

SchemaParser::Error::Error(const std::string& what, size_t line) : eckit::StreamParser::Error(what, line) {
if (line) {
std::ostringstream oss;
oss << "Line: " << line << " " << what;
reason(oss.str());
}
}

} // namespace fdb5
18 changes: 16 additions & 2 deletions src/fdb5/rules/SchemaParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
#ifndef fdb5_SchemaParser_h
#define fdb5_SchemaParser_h

#include <cstddef>
#include <iosfwd>
#include <memory>
#include <string>
Comment thread
tbkr marked this conversation as resolved.

#include "eckit/exception/Exceptions.h"
#include "eckit/parser/StreamParser.h"
#include "eckit/types/Types.h"

Expand All @@ -27,16 +29,28 @@ namespace fdb5 {

//----------------------------------------------------------------------------------------------------------------------

class SchemaParser : public eckit::StreamParser {
class SchemaParser {

public: // methods

SchemaParser(std::istream& in) : StreamParser(in, true) {}
class Error : public eckit::StreamParser::Error {
public:

Error(const std::string& what, size_t line = 0);
};

SchemaParser(std::istream& in) : parser(eckit::StreamParser(in)) {}

void parse(RuleList& result, TypesRegistry& registry);

private: // methods

bool isAscii(char c);

char peek(bool spaces = false);

eckit::StreamParser parser;
Comment thread
tbkr marked this conversation as resolved.
Outdated

std::string parseIdent(bool value, bool emptyOK);

std::unique_ptr<RuleDatum> parseDatum();
Expand Down
1 change: 1 addition & 0 deletions tests/fdb/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ add_subdirectory( api )
add_subdirectory( database )
add_subdirectory( type )
add_subdirectory( daos )
add_subdirectory( parsing )

if (HAVE_FDB_BUILD_TOOLS)
add_subdirectory( timespan )
Expand Down
13 changes: 13 additions & 0 deletions tests/fdb/parsing/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
set( _test_environment ${test_environment})

list( APPEND _test_environment
FDB_HOME=${PROJECT_BINARY_DIR} )

file(COPY data DESTINATION ${CMAKE_CURRENT_BINARY_DIR})

ecbuild_add_test( TARGET fdb_test_schema_parsing
SOURCES
test_schema_parsing.cc
LIBS
fdb5
ENVIRONMENT "${_test_environment}")
6 changes: 6 additions & 0 deletions tests/fdb/parsing/data/broken_schema_no_rule
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
param: Param;
step: Step;
date: Date;
levelist: Double;
expver: Expver;
time: Time;
Loading
Loading