Skip to content
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

[7392] [cpp] Remove dead code path for map key of type enum in JSON parsing #16567

Closed

Conversation

antongrbin
Copy link
Contributor

@antongrbin antongrbin commented Apr 18, 2024

Changes

Remove dead code path -- we don't allow enums to be map keys (proto2 spec, proto3 spec). In other words the case block case FieldDescriptor::TYPE_ENUM is dead code. Potential enum type keys will be caught in default: return lex.Invalid("unsupported map key type"); block below similar to other unsupported map key types like double.

Motivation

While working on fixing IgnoreUnknownEnumStringValueInMap conformance tests for cpp (related issue) I stumbled upon a bug where we pass the wrong field parameter to the enum parsing function.

In this scope:

  • the variable field is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires.
  • the variable key_field is the key field of the map message entry. This field is the enum type that we need to parse here.

The function is long, so I clarified it here:

template <typename Traits>
absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) {
  (..)
  return lex.VisitObject(
      [&](LocationWith<MaybeOwnedString>& key) -> absl::Status {
          (..)
          return Traits::NewMsg(
            field, msg,
            [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
              auto key_field = Traits::KeyField(type);
              switch (Traits::FieldType(key_field)) {
                (..)
                case FieldDescriptor::TYPE_ENUM: {
                  MaybeOwnedString key_str = key.value;
                  auto e = ParseEnumFromStr<Traits>(lex, key_str, /** bug here **/ field);

The correct reference should be key_field.

Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether.

@antongrbin antongrbin changed the title [7392] Bugfix in JSON map key parsing dead code [7392] [bugfix in dead code] [cpp] Fix JSON map key parsing field reference Apr 18, 2024
@antongrbin antongrbin changed the title [7392] [bugfix in dead code] [cpp] Fix JSON map key parsing field reference [7392] [cpp] Fix JSON map key parsing field reference (in dead code) Apr 18, 2024
@antongrbin antongrbin marked this pull request as ready for review April 18, 2024 19:42
@antongrbin antongrbin requested a review from a team as a code owner April 18, 2024 19:42
@antongrbin antongrbin requested review from sbenzaquen and removed request for a team April 18, 2024 19:42
@antongrbin
Copy link
Contributor Author

@sbenzaquen friendly ping -- please advise if we should continue with the bugfix or remove the dead code block.

@antongrbin
Copy link
Contributor Author

@jskeet, I am reaching out to you since you worked with me on #7392 and this PR is first in line to fix the remaining corner case for C++.

Can you help me reach @sbenzaquen -- I suspect they are not seeing github notifications here.

Btw, nothing urgent here, I am not blocked on this -- just minimizng the amount of things I have open.

@jskeet
Copy link
Contributor

jskeet commented May 8, 2024

Have sent a ping by internal email.

@sbenzaquen
Copy link
Contributor

Sorry for the delay.
Given that this code path is dead and it can't be tested, I think a better approach is to remove the ENUM case as you mention. If we ever start supporting this, I rather we write the code at that time when unittests are added instead of silently turning on code that has never run.
We already do not handle other invalid key types (like double) so this type should go into that defualt: case too.

@antongrbin antongrbin changed the title [7392] [cpp] Fix JSON map key parsing field reference (in dead code) [7392] [cpp] Remove dead code path for map key of type enum in JSON parsing May 9, 2024
@antongrbin
Copy link
Contributor Author

@sbenzaquen -- thank you! Updated the PR accordingly.

@sbenzaquen sbenzaquen added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 10, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 10, 2024
@antongrbin
Copy link
Contributor Author

@sbenzaquen thank you for the review! There was (I believe) an unrelated test failure that blocked merge:
https://github.com/protocolbuffers/protobuf/actions/runs/9036284406/job/24832883483#step:3:909

I merged with latest main to see if it's still there. I need another "safe for test" label.

@honglooker honglooker added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2024
copybara-service bot pushed a commit that referenced this pull request May 16, 2024
…arsing (#16567)

# Changes

Remove dead code path -- we don't allow enums to be map keys ([proto2 spec](https://protobuf.dev/programming-guides/proto2/#maps), [proto3 spec](https://protobuf.dev/programming-guides/proto3/#maps)). In other words the case block `case FieldDescriptor::TYPE_ENUM` is dead code. Potential enum type keys will be caught in `default: return lex.Invalid("unsupported map key type");` block below similar to other unsupported map key types like double.

# Motivation

While working on fixing `IgnoreUnknownEnumStringValueInMap` conformance tests for cpp ([related issue](#7392)) I stumbled upon a bug where we pass the wrong `field` parameter to the enum parsing function.

In this scope:
* the variable `field` is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires.
* the variable `key_field` is the key field of the map message entry. This field is the enum type that we need to parse here.

The function is long, so I clarified it here:

```cpp
template <typename Traits>
absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) {
  (..)
  return lex.VisitObject(
      [&](LocationWith<MaybeOwnedString>& key) -> absl::Status {
          (..)
          return Traits::NewMsg(
            field, msg,
            [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
              auto key_field = Traits::KeyField(type);
              switch (Traits::FieldType(key_field)) {
                (..)
                case FieldDescriptor::TYPE_ENUM: {
                  MaybeOwnedString key_str = key.value;
                  auto e = ParseEnumFromStr<Traits>(lex, key_str, /** bug here **/ field);
```

The correct reference should be `key_field`.

Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether.

Closes #16567

COPYBARA_INTEGRATE_REVIEW=#16567 from noom:anton--7392--fix-map-key-nit d992b8a
FUTURE_COPYBARA_INTEGRATE_REVIEW=#16567 from noom:anton--7392--fix-map-key-nit d992b8a
PiperOrigin-RevId: 634509516
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this pull request Jun 20, 2024
…arsing (protocolbuffers#16567)

# Changes

Remove dead code path -- we don't allow enums to be map keys ([proto2 spec](https://protobuf.dev/programming-guides/proto2/#maps), [proto3 spec](https://protobuf.dev/programming-guides/proto3/#maps)). In other words the case block `case FieldDescriptor::TYPE_ENUM` is dead code. Potential enum type keys will be caught in `default: return lex.Invalid("unsupported map key type");` block below similar to other unsupported map key types like double.

# Motivation

While working on fixing `IgnoreUnknownEnumStringValueInMap` conformance tests for cpp ([related issue](protocolbuffers#7392)) I stumbled upon a bug where we pass the wrong `field` parameter to the enum parsing function.

In this scope:
* the variable `field` is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires.
* the variable `key_field` is the key field of the map message entry. This field is the enum type that we need to parse here.

The function is long, so I clarified it here:

```cpp
template <typename Traits>
absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) {
  (..)
  return lex.VisitObject(
      [&](LocationWith<MaybeOwnedString>& key) -> absl::Status {
          (..)
          return Traits::NewMsg(
            field, msg,
            [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
              auto key_field = Traits::KeyField(type);
              switch (Traits::FieldType(key_field)) {
                (..)
                case FieldDescriptor::TYPE_ENUM: {
                  MaybeOwnedString key_str = key.value;
                  auto e = ParseEnumFromStr<Traits>(lex, key_str, /** bug here **/ field);
```

The correct reference should be `key_field`.

Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether.

Closes protocolbuffers#16567

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#16567 from noom:anton--7392--fix-map-key-nit d992b8a
PiperOrigin-RevId: 634516984
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants