Skip to content

Commit 6f5706f

Browse files
g31pranjalg31pranjal
authored
Omit __ prefixed identifiers from protobuf translation (#3706)
This PR builds on top of #3696 and does 2 things - Omits `__` prefixed identifiers from getting translated to protobuf compliant name altogether. - Checks if the incoming identifier, if or if not translated, is protobuf compliant, else throws an "INVALID_NAME" exception There are 2 more things covered: - Ensures view name is protobuf compliant - Adds a bunch of negative tests around having invalid identifiers
1 parent 8ebb23e commit 6f5706f

File tree

8 files changed

+378
-47
lines changed

8 files changed

+378
-47
lines changed

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/metadata/DataTypeUtils.java‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.Locale;
3535
import java.util.Optional;
3636
import java.util.UUID;
37+
import java.util.regex.Pattern;
3738
import java.util.stream.Collectors;
3839

3940
@API(API.Status.EXPERIMENTAL)
@@ -43,6 +44,10 @@ public class DataTypeUtils {
4344
private static final String DOLLAR_ESCAPE = "__1";
4445
private static final String DOT_ESCAPE = "__2";
4546

47+
private static final List<String> INVALID_START_SEQUENCES = List.of(".", "$", DOUBLE_UNDERSCORE_ESCAPE, DOLLAR_ESCAPE, DOT_ESCAPE);
48+
49+
private static final Pattern VALID_PROTOBUF_COMPLIANT_NAME_PATTERN = Pattern.compile("^[A-Za-z_][A-Za-z0-9_]*$");
50+
4651
@Nonnull
4752
private static final BiMap<DataType, Type> primitivesMap;
4853

@@ -121,10 +126,29 @@ private static String getUniqueName() {
121126
return "id" + modified;
122127
}
123128

124-
public static String toProtoBufCompliantName(String userIdentifier) {
129+
@Nonnull
130+
public static String toProtoBufCompliantName(final String name) {
131+
Assert.thatUnchecked(INVALID_START_SEQUENCES.stream().noneMatch(name::startsWith), ErrorCode.INVALID_NAME, "name cannot start with %s", INVALID_START_SEQUENCES);
132+
String translated;
133+
if (name.startsWith("__")) {
134+
translated = "__" + translateSpecialCharacters(name.substring(2));
135+
} else {
136+
Assert.thatUnchecked(!name.isEmpty(), ErrorCode.INVALID_NAME, "name cannot be empty String.");
137+
translated = translateSpecialCharacters(name);
138+
}
139+
checkValidProtoBufCompliantName(translated);
140+
return translated;
141+
}
142+
143+
@Nonnull
144+
private static String translateSpecialCharacters(final String userIdentifier) {
125145
return userIdentifier.replace("__", DOUBLE_UNDERSCORE_ESCAPE).replace("$", DOLLAR_ESCAPE).replace(".", DOT_ESCAPE);
126146
}
127147

148+
public static void checkValidProtoBufCompliantName(String name) {
149+
Assert.thatUnchecked(VALID_PROTOBUF_COMPLIANT_NAME_PATTERN.matcher(name).matches(), ErrorCode.INVALID_NAME, name + " is not a valid name!");
150+
}
151+
128152
public static String toUserIdentifier(String protoIdentifier) {
129153
return protoIdentifier.replace(DOT_ESCAPE, ".").replace(DOLLAR_ESCAPE, "$").replace(DOUBLE_UNDERSCORE_ESCAPE, "__");
130154
}

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Identifier.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,7 @@ public boolean qualifiedWith(@Nonnull Identifier identifier) {
147147
@Nonnull
148148
public static Identifier toProtobufCompliant(@Nonnull final Identifier identifier) {
149149
final var qualifier = identifier.getQualifier().stream().map(DataTypeUtils::toProtoBufCompliantName).collect(Collectors.toList());
150-
final var name = DataTypeUtils.toProtoBufCompliantName(identifier.getName());
151-
return Identifier.of(name, qualifier);
150+
return Identifier.of(DataTypeUtils.toProtoBufCompliantName(identifier.getName()), qualifier);
152151
}
153152

154153
@Override

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/PseudoColumn.java‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,4 @@ public static Optional<Expression> mapToExpressionMaybe(@Nonnull LogicalOperator
7373
}
7474
return Optional.empty();
7575
}
76-
77-
public static boolean isPseudoColumn(@Nonnull String name) {
78-
for (PseudoColumn pseudo : PseudoColumn.values()) {
79-
if (name.equals(pseudo.getColumnName())) {
80-
return true;
81-
}
82-
}
83-
return false;
84-
}
8576
}

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/DdlVisitor.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ private RecordLayerView getViewMetadata(@Nonnull final RelationalParser.ViewDefi
379379
getDelegate().replaceSchemaTemplate(ddlCatalog);
380380

381381
// 1. get the view name.
382-
final var viewName = visitFullId(viewCtx.viewName).toString();
382+
final var viewName = Identifier.toProtobufCompliant(visitFullId(viewCtx.viewName)).getName();
383383

384384
// 2. get the view SQL definition string.
385385
final var queryString = getDelegate().getPlanGenerationContext().getQuery();

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/ExpressionVisitor.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@
4848
import com.apple.foundationdb.relational.recordlayer.query.LogicalPlanFragment;
4949
import com.apple.foundationdb.relational.recordlayer.query.OrderByExpression;
5050
import com.apple.foundationdb.relational.recordlayer.query.ParseHelpers;
51-
import com.apple.foundationdb.relational.recordlayer.query.PseudoColumn;
5251
import com.apple.foundationdb.relational.recordlayer.query.SemanticAnalyzer;
5352
import com.apple.foundationdb.relational.recordlayer.query.StringTrieNode;
5453
import com.apple.foundationdb.relational.recordlayer.query.TautologicalValue;
@@ -181,11 +180,7 @@ public Expression visitSelectExpressionElement(@Nonnull RelationalParser.SelectE
181180
@Override
182181
public Expression visitFullColumnName(@Nonnull RelationalParser.FullColumnNameContext fullColumnNameContext) {
183182
final var id = visitFullId(fullColumnNameContext.fullId());
184-
if (!PseudoColumn.isPseudoColumn(id.getName())) {
185-
return getDelegate().getSemanticAnalyzer().resolveIdentifier(Identifier.toProtobufCompliant(id), getDelegate().getCurrentPlanFragment());
186-
} else {
187-
return getDelegate().getSemanticAnalyzer().resolveIdentifier(id.replaceQualifier(q -> q.stream().map(DataTypeUtils::toProtoBufCompliantName).collect(Collectors.toList())), getDelegate().getCurrentPlanFragment());
188-
}
183+
return getDelegate().getSemanticAnalyzer().resolveIdentifier(Identifier.toProtobufCompliant(id), getDelegate().getCurrentPlanFragment());
189184
}
190185

191186
@Nonnull

‎fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/metadata/DataTypeUtilsTest.java‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/*
2+
* DataTypeUtilsTest.java
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2015-2025 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
21+
package com.apple.foundationdb.relational.recordlayer.metadata;
22+
23+
import com.apple.foundationdb.relational.api.exceptions.UncheckedRelationalException;
24+
import org.assertj.core.api.Assertions;
25+
import org.junit.jupiter.params.ParameterizedTest;
26+
import org.junit.jupiter.params.provider.Arguments;
27+
import org.junit.jupiter.params.provider.MethodSource;
28+
29+
import javax.annotation.Nonnull;
30+
import javax.annotation.Nullable;
31+
import java.util.stream.Stream;
32+
33+
public class DataTypeUtilsTest {
34+
35+
36+
static Stream<Arguments> protobufCompliantNameTestArguments() {
37+
return Stream.of(
38+
Arguments.of("__", "__"),
39+
Arguments.of("_", "_"),
40+
Arguments.of("$", null),
41+
Arguments.of(".", null),
42+
Arguments.of("__hello", "__hello"),
43+
Arguments.of("__$hello", "____1hello"),
44+
Arguments.of("__$hello", "____1hello"),
45+
Arguments.of("__.hello", "____2hello"),
46+
Arguments.of("____hello", "____0hello"),
47+
Arguments.of("__$h$e$l$l$o", "____1h__1e__1l__1l__1o"),
48+
Arguments.of("__0", null),
49+
Arguments.of("__0hello", null),
50+
Arguments.of("__1", null),
51+
Arguments.of("__1hello", null),
52+
Arguments.of("__2", null),
53+
Arguments.of("__2hello", null),
54+
Arguments.of("__4", "__4"),
55+
Arguments.of("__$$$$", "____1__1__1__1"),
56+
Arguments.of("______", "____0__0"),
57+
Arguments.of("__4hello", "__4hello"),
58+
Arguments.of("$", null),
59+
Arguments.of("$hello", null),
60+
Arguments.of(".", null),
61+
Arguments.of(".hello", null),
62+
Arguments.of("h__e__l__l__o", "h__0e__0l__0l__0o"),
63+
Arguments.of("h.e.l.l.o", "h__2e__2l__2l__2o"),
64+
Arguments.of("h$e$l$l$o", "h__1e__1l__1l__1o"),
65+
Arguments.of("1hello", null),
66+
Arguments.of("डेटाबेस", null)
67+
);
68+
}
69+
70+
@ParameterizedTest
71+
@MethodSource("protobufCompliantNameTestArguments")
72+
void protobufCompliantNameTest(@Nonnull String userIdentifier, @Nullable String protobufIdentifier) {
73+
if (protobufIdentifier != null) {
74+
final var actual = DataTypeUtils.toProtoBufCompliantName(userIdentifier);
75+
Assertions.assertThat(actual).isEqualTo(protobufIdentifier);
76+
final var reTranslated = DataTypeUtils.toUserIdentifier(actual);
77+
Assertions.assertThat(reTranslated).isEqualTo(userIdentifier);
78+
} else {
79+
Assertions.assertThatThrownBy(() -> DataTypeUtils.toProtoBufCompliantName(userIdentifier))
80+
.isInstanceOf(UncheckedRelationalException.class);
81+
}
82+
}
83+
}

‎yaml-tests/src/test/proto/identifiers.proto‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/*
2+
* identifiers.proto
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2021-2024 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
21+
syntax = "proto3";
22+
23+
// make sure to use this package naming convention:
24+
// com.apple.foundationdb.relational.yamltests.generated.<name_of_test_suite>
25+
// adding "generated" is important to exclude the generated Java file from Jacoco reports.
26+
// suffixing the namespace with your test name is important for segregating similarly-named
27+
// generated-Java-classes (such as RecordTypeUnion) into separate packages, otherwise you
28+
// get an error from `protoc`.
29+
package com.apple.foundationdb.relational.yamltests.generated.identifierstests;
30+
31+
option java_outer_classname = "IdentifiersTestProto";
32+
33+
import "record_metadata_options.proto";
34+
35+
message T1 {
36+
int64 ID = 1 [(com.apple.foundationdb.record.field).primary_key = true];
37+
int64 T1__2COL1 = 2;
38+
int64 T1__2COL2 = 3;
39+
}
40+
41+
message T2 {
42+
int64 ID = 1 [(com.apple.foundationdb.record.field).primary_key = true];
43+
int64 T2__1COL1 = 2;
44+
int64 T2__1COL2 = 3;
45+
}
46+
47+
message __T3 {
48+
int64 ID = 1 [(com.apple.foundationdb.record.field).primary_key = true];
49+
int64 __T3__1COL1 = 2;
50+
int64 __T3__1COL2 = 3;
51+
}
52+
53+
message internal {
54+
int64 a = 1;
55+
int64 b = 2;
56+
}
57+
58+
message T4 {
59+
int64 ID = 1 [(com.apple.foundationdb.record.field).primary_key = true];
60+
internal ___hidden = 2;
61+
int64 T4__2COL1 = 3;
62+
int64 T4__2COL2 = 4;
63+
}
64+
65+
message RecordTypeUnion {
66+
T1 _T1 = 1;
67+
T2 _T2 = 2;
68+
__T3 ___T3 = 3;
69+
T4 _T4 = 4;
70+
}

0 commit comments

Comments
 (0)