Skip to content

Commit 336c01a

Browse files
Shen Yumeta-codesync[bot]
Shen Yu
authored and
meta-codesync[bot]
committed
Displaying the thrift name in the InvalidTypeError
Summary: Now if we call `apache::thrift::SchemaRegistry::get().getTypeSystemNod` on a thrift type, and its or its dependent's thrift definition is missing the package name, the error is not telling which thrift definition is missing the package. This diff adds the thrift definition's name to the error message. Reviewed By: praihan Differential Revision: D94431637 fbshipit-source-id: 2546f64225ef3cfb0c2aa32aff7297f908860b55
1 parent 2313edd commit 336c01a

File tree

8 files changed

+391
-42
lines changed

8 files changed

+391
-42
lines changed

‎thrift/lib/cpp2/dynamic/TypeSystem.cpp‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,9 @@ StructuredNode::StructuredNode(
9191
Uri uri,
9292
std::vector<FieldDefinition> fields,
9393
bool isSealed,
94-
AnnotationsMap annotations)
95-
: DefinitionNode(std::move(uri)),
94+
AnnotationsMap annotations,
95+
std::string debugName)
96+
: DefinitionNode(std::move(uri), std::move(debugName)),
9697
fields_(std::move(fields)),
9798
isSealed_(isSealed),
9899
annotations_(std::move(annotations)) {
@@ -127,18 +128,27 @@ StructNode::StructNode(
127128
Uri uri,
128129
std::vector<FieldDefinition> fields,
129130
bool isSealed,
130-
AnnotationsMap annotations)
131+
AnnotationsMap annotations,
132+
std::string debugName)
131133
: StructuredNode(
132-
std::move(uri), std::move(fields), isSealed, std::move(annotations)) {
133-
}
134+
std::move(uri),
135+
std::move(fields),
136+
isSealed,
137+
std::move(annotations),
138+
std::move(debugName)) {}
134139

135140
UnionNode::UnionNode(
136141
Uri uri,
137142
std::vector<FieldDefinition> fields,
138143
bool isSealed,
139-
AnnotationsMap annotations)
144+
AnnotationsMap annotations,
145+
std::string debugName)
140146
: StructuredNode(
141-
std::move(uri), std::move(fields), isSealed, std::move(annotations)) {
147+
std::move(uri),
148+
std::move(fields),
149+
isSealed,
150+
std::move(annotations),
151+
std::move(debugName)) {
142152
for (const FieldDefinition& field : this->fields()) {
143153
if (field.presence() != PresenceQualifier::OPTIONAL_) {
144154
throw InvalidTypeError(
@@ -348,7 +358,12 @@ std::string_view kindToString(TypeRef::Kind k) noexcept {
348358

349359
const Uri& DefinitionNode::uri() const {
350360
if (uri_.empty()) {
351-
throw InvalidTypeError("Type does not have URI set.");
361+
throw InvalidTypeError(
362+
fmt::format(
363+
"Type `{}` does not have URI set."
364+
" The thrift file may be missing a package declaration,"
365+
" see https://fburl.com/thrift-uri-add-package",
366+
debugName_));
352367
}
353368
return uri_;
354369
}

‎thrift/lib/cpp2/dynamic/TypeSystem.h‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -981,10 +981,22 @@ class DefinitionNode {
981981
*/
982982
const Uri& uri() const;
983983

984+
/**
985+
* Produces a non-unique human-readable name, if one is available.
986+
* Otherwise, this is an empty string.
987+
*
988+
* This name MUST NOT be used to identify types. It should only be
989+
* used for diagnostic messages or debugging. There is no validation
990+
* or guarantees around the format of the returned string.
991+
*/
992+
std::string_view debugName() const noexcept { return debugName_; }
993+
984994
protected:
985995
Uri uri_;
996+
std::string debugName_;
986997

987-
explicit DefinitionNode(Uri uri) noexcept : uri_(std::move(uri)) {}
998+
explicit DefinitionNode(Uri uri, std::string debugName) noexcept
999+
: uri_(std::move(uri)), debugName_(std::move(debugName)) {}
9881000
};
9891001

9901002
class StructuredNode : public DefinitionNode {
@@ -1074,7 +1086,8 @@ class StructuredNode : public DefinitionNode {
10741086
Uri uri,
10751087
std::vector<FieldDefinition> fields,
10761088
bool isSealed,
1077-
AnnotationsMap annotations);
1089+
AnnotationsMap annotations,
1090+
std::string debugName);
10781091
};
10791092

10801093
class StructNode final : folly::MoveOnly, public StructuredNode {
@@ -1083,7 +1096,8 @@ class StructNode final : folly::MoveOnly, public StructuredNode {
10831096
Uri,
10841097
std::vector<FieldDefinition>,
10851098
bool isSealed,
1086-
AnnotationsMap annotations);
1099+
AnnotationsMap annotations,
1100+
std::string debugName);
10871101

10881102
TypeRef asRef() const noexcept { return TypeRef(*this); }
10891103
};
@@ -1094,7 +1108,8 @@ class UnionNode final : folly::MoveOnly, public StructuredNode {
10941108
Uri,
10951109
std::vector<FieldDefinition>,
10961110
bool isSealed,
1097-
AnnotationsMap annotations);
1111+
AnnotationsMap annotations,
1112+
std::string debugName);
10981113

10991114
TypeRef asRef() const noexcept { return TypeRef(*this); }
11001115
};
@@ -1129,8 +1144,11 @@ class EnumNode final : folly::MoveOnly, public DefinitionNode {
11291144
}
11301145

11311146
explicit EnumNode(
1132-
Uri uri, std::vector<Value> values, AnnotationsMap annotations)
1133-
: DefinitionNode(std::move(uri)),
1147+
Uri uri,
1148+
std::vector<Value> values,
1149+
AnnotationsMap annotations,
1150+
std::string debugName)
1151+
: DefinitionNode(std::move(uri), std::move(debugName)),
11341152
values_(std::move(values)),
11351153
annotations_(std::move(annotations)) {}
11361154

@@ -1153,8 +1171,11 @@ class OpaqueAliasNode final : folly::MoveOnly, public DefinitionNode {
11531171
}
11541172

11551173
explicit OpaqueAliasNode(
1156-
Uri uri, TypeRef targetType, AnnotationsMap annotations)
1157-
: DefinitionNode(std::move(uri)),
1174+
Uri uri,
1175+
TypeRef targetType,
1176+
AnnotationsMap annotations,
1177+
std::string debugName)
1178+
: DefinitionNode(std::move(uri), std::move(debugName)),
11581179
targetType_(targetType),
11591180
annotations_(std::move(annotations)) {}
11601181

‎thrift/lib/cpp2/dynamic/TypeSystemBuilder.cpp‎

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -264,16 +264,19 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
264264
// Fill in definitions with uninitialized stubs
265265
for (auto& [uri, entry] : definitions_) {
266266
SerializableTypeDefinition& def = entry.definition;
267+
std::string defName = entry.sourceInfo.has_value()
268+
? std::string(*entry.sourceInfo->name())
269+
: std::string{};
267270
auto uninitDef = std::invoke([&]() -> TSDefinition {
268271
switch (def.getType()) {
269272
case SerializableTypeDefinition::Type::structDef:
270-
return StructNode{uri, {}, {}, {}};
273+
return StructNode{uri, {}, {}, {}, defName};
271274
case SerializableTypeDefinition::Type::unionDef:
272-
return UnionNode{uri, {}, {}, {}};
275+
return UnionNode{uri, {}, {}, {}, defName};
273276
case SerializableTypeDefinition::Type::enumDef:
274-
return EnumNode{uri, {}, {}};
277+
return EnumNode{uri, {}, {}, defName};
275278
case SerializableTypeDefinition::Type::opaqueAliasDef:
276-
return OpaqueAliasNode{uri, TypeRef{TypeRef::Bool{}}, {}};
279+
return OpaqueAliasNode{uri, TypeRef{TypeRef::Bool{}}, {}, defName};
277280
default:
278281
break;
279282
}
@@ -333,6 +336,9 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
333336
std::optional<SerializableThriftSourceInfo>& sourceInfo = entry.sourceInfo;
334337
// We created uninitialized stubs above so we can assume they exist
335338
TSDefinition& uninitDef = typeSystem->definitions.find(uri)->second;
339+
std::string defName = sourceInfo.has_value()
340+
? std::string(*sourceInfo->name())
341+
: std::string{};
336342

337343
switch (def.getType()) {
338344
case SerializableTypeDefinition::Type::structDef: {
@@ -342,7 +348,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
342348
uri,
343349
makeFields(std::move(*structDef.fields())),
344350
*structDef.isSealed(),
345-
makeAnnots(std::move(*structDef.annotations())));
351+
makeAnnots(std::move(*structDef.annotations())),
352+
defName);
346353
if (sourceInfo.has_value()) {
347354
typeSystem->tryAddToSourceIndex(
348355
SourceIdentifier{*sourceInfo->locator(), *sourceInfo->name()},
@@ -356,7 +363,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
356363
uri,
357364
makeFields(std::move(*unionDef.fields())),
358365
*unionDef.isSealed(),
359-
makeAnnots(std::move(*unionDef.annotations())));
366+
makeAnnots(std::move(*unionDef.annotations())),
367+
defName);
360368
if (sourceInfo.has_value()) {
361369
typeSystem->tryAddToSourceIndex(
362370
SourceIdentifier{*sourceInfo->locator(), *sourceInfo->name()},
@@ -378,7 +386,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
378386
enumNode = EnumNode(
379387
uri,
380388
std::move(values),
381-
makeAnnots(std::move(*enumDef.annotations())));
389+
makeAnnots(std::move(*enumDef.annotations())),
390+
defName);
382391
if (sourceInfo.has_value()) {
383392
typeSystem->tryAddToSourceIndex(
384393
SourceIdentifier{*sourceInfo->locator(), *sourceInfo->name()},
@@ -392,7 +401,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
392401
opaqueAliasNode = OpaqueAliasNode(
393402
uri,
394403
typeSystem->typeOf(*opaqueAliasDef.targetType()),
395-
makeAnnots(std::move(*opaqueAliasDef.annotations())));
404+
makeAnnots(std::move(*opaqueAliasDef.annotations())),
405+
defName);
396406
if (sourceInfo.has_value()) {
397407
typeSystem->tryAddToSourceIndex(
398408
SourceIdentifier{*sourceInfo->locator(), *sourceInfo->name()},
@@ -425,15 +435,18 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
425435
for (const auto& uri : uris) {
426436
auto sourceDef = source.getUserDefinedType(uri);
427437
auto stubDef = sourceDef->visit(
428-
[&](const StructNode&) -> TSDefinition {
429-
return StructNode{uri, {}, false, {}};
438+
[&](const StructNode& node) -> TSDefinition {
439+
return StructNode{uri, {}, false, {}, std::string(node.debugName())};
430440
},
431-
[&](const UnionNode&) -> TSDefinition {
432-
return UnionNode{uri, {}, false, {}};
441+
[&](const UnionNode& node) -> TSDefinition {
442+
return UnionNode{uri, {}, false, {}, std::string(node.debugName())};
433443
},
434-
[&](const EnumNode&) -> TSDefinition { return EnumNode{uri, {}, {}}; },
435-
[&](const OpaqueAliasNode&) -> TSDefinition {
436-
return OpaqueAliasNode{uri, TypeRef{TypeRef::Bool{}}, {}};
444+
[&](const EnumNode& node) -> TSDefinition {
445+
return EnumNode{uri, {}, {}, std::string(node.debugName())};
446+
},
447+
[&](const OpaqueAliasNode& node) -> TSDefinition {
448+
return OpaqueAliasNode{
449+
uri, TypeRef{TypeRef::Bool{}}, {}, std::string(node.debugName())};
437450
});
438451
typeSystem->definitions.emplace(uri, std::move(stubDef));
439452
}
@@ -484,7 +497,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
484497
uri,
485498
copyFields(node.fields()),
486499
node.isSealed(),
487-
copyAnnotations(node.annotations()));
500+
copyAnnotations(node.annotations()),
501+
std::string(node.debugName()));
488502
if (options.includeSourceInfo) {
489503
auto sourceInfo =
490504
source.getSourceIdentiferForUserDefinedType(*sourceDef);
@@ -503,7 +517,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
503517
uri,
504518
copyFields(node.fields()),
505519
node.isSealed(),
506-
copyAnnotations(node.annotations()));
520+
copyAnnotations(node.annotations()),
521+
std::string(node.debugName()));
507522
if (options.includeSourceInfo) {
508523
auto sourceInfo =
509524
source.getSourceIdentiferForUserDefinedType(*sourceDef);
@@ -526,7 +541,10 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
526541
}
527542
auto& enumNode = std::get<EnumNode>(stubDef);
528543
enumNode = EnumNode(
529-
uri, std::move(values), copyAnnotations(node.annotations()));
544+
uri,
545+
std::move(values),
546+
copyAnnotations(node.annotations()),
547+
std::string(node.debugName()));
530548
if (options.includeSourceInfo) {
531549
auto sourceInfo =
532550
source.getSourceIdentiferForUserDefinedType(*sourceDef);
@@ -544,7 +562,8 @@ std::unique_ptr<TypeSystem> TypeSystemBuilder::buildDerivedFrom(
544562
opaqueAliasNode = OpaqueAliasNode(
545563
uri,
546564
remapType(node.targetType()),
547-
copyAnnotations(node.annotations()));
565+
copyAnnotations(node.annotations()),
566+
std::string(node.debugName()));
548567
if (options.includeSourceInfo) {
549568
auto sourceInfo =
550569
source.getSourceIdentiferForUserDefinedType(*sourceDef);

0 commit comments

Comments
 (0)