grpc-ecosystem/grpc-gateway

Inconsistency in handling null in message vs map<string, message> in PATCH parsing

Open

#2,056 opened on Mar 25, 2021

View on GitHub
 (2 comments) (1 reaction) (0 assignees)Go (16,971 stars) (2,250 forks)batch import
bughelp wanted

Description

🐛 Bug Report

Null value is accepted for message-type field but not for same type map field.

To Reproduce

syntax = "proto3";
...
service SomeService {
    rpc UpdateData(UpdateDataRequest) returns (Data) {
        option (google.api.http).patch = "/";
        option (google.api.http).body = "data";
    }
}
message UpdateDataRequest {
    Data data = 1;
    google.protobuf.FieldMask update_mask = 2;
}
message Data {
    google.protobuf.Duration wkt = 1;
    Duration dur = 2;
    map<string, google.protobuf.Duration> mapwkt = 3;
    map<string, Duration> mapdur = 4;
}
message Duration { ... same as google.protobuf.Duration ... }

Expected behavior

I expect ability to send same value no matter is it a message-type field or map-of-messages-type field.

Actual Behavior

The problem is difference between 1 vs 5, and 3 vs 7 - I think null values should be either accepted or rejected in all cases.

  1. {"wkt":null} ⇒ update_mask:{paths:"wkt"}
  2. {"wkt":"0s"} ⇒ update_mask:{paths:"wkt"} data:{wkt:{}}
  3. {"dur":null} ⇒ update_mask:{paths:"dur"}
  4. {"dur":{}} ⇒ update_mask:{} data:{dur:{}}
  5. {"mapwkt":{"key":null}} ⇒ ERROR: unexpected token null
  6. {"mapwkt":{"key":"0s"}} ⇒ update_mask:{paths:"mapwkt"} data:{mapwkt:{key:"key" value:{}}}
  7. {"mapdur":{"key":null}} ⇒ ERROR: unexpected token null
  8. {"mapdur":{"key":{}}} ⇒ update_mask:{paths:"mapdur"} data:{mapdur:{key:"key" value:{}}}

Also 3 vs 4 looks a bit weird, not sure is it correct or not and if not is it related to the issue.

Your Environment

Linux, go1.16.2, grpc-gateway v2.3.0.

Contributor guide

Tech stack
go
Domain
api
Issue type
bug
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
3
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
half day
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
gRPCprotobufJSON parsing
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
60
Research direction
Investigate the PATCH parsing logic in grpc gateway, specifically how JSON body is unmarshalled and field masks are applied. Look at the code for handling map<string, message> fields where the value is null. Check if the issue is in the protobuf JSON library or in the gateway's custom logic. Examine existing tests for map fields with null values and consider adding a test case for this inconsistency.
Inconsistency in handling null in message vs map<string, message> in PATCH parsing · grpc-ecosystem/grpc-gateway#2056 | Good First Issue