[FeatureRequest] Have option to generate completed object with nested object

[tnhh00037]

Current behavior

With message like this

message Information {
    string name = 1;
    number age = 2;
    Detail detail = 3;

    message Detail {
       string address = 1;
    }
}

Using create or fromPartial method will not assign value for detail field (undefined)

const obj = Information.create({});
// obj: { name :"" , age: 0 , detail : undefined }

To fix that, user needs to do like below. If object has deeper nested field, it's going to be requiring a lot of manual work.

const detail = Information_Detail.create();
const obj = Information.create({detail})
// obj: { name :"" , age: 0 , detail : { address: ""  } }

Suggestion!!

Have option to let user create completed object with nested level.

const obj = Information.create({});
// obj: { name :"" , age: 0 , detail : { address: ""  } }

With this ability user can create object for unit test + main logic without needing to create separated object for each nested field. I recommend adding flag --ts_proto_opt=defaultNestedField=true
Then in src/main.ts

// Old 
function generateFromPartial(ctx: Context, fullName: string, messageDesc: DescriptorProto): Code {
    ...
    // line 2246
    } else {
          const fallback = isWithinOneOf(field) || noDefaultValue ? "undefined" : defaultValue(ctx, field);
          chunks.push(code`
            message.${fieldName} = (object.${fieldName} !== undefined && object.${fieldName} !== null)
              ? ${readSnippet(`object.${fieldName}`)}
              : ${fallback};
          `);
        }
    
    // => Change to:
        } else {
            const autoAssignNestedField = ctx.autoAssignNestedField === true;
            const fallback = isWithinOneOf(field) || noDefaultValue ? "undefined" : defaultValue(ctx, field);

            chunks.push(
             autoAssignNestedField ? 
                // if object.${fieldName} doesn't exist, fallback to empty object to allow creating Nested Object by passing {} to its `fromPartial`
                 code`
                    message.${fieldName} = ${readSnippet(`object?.${fieldName} || {}`)} 
                `) 
                 : code`
                    message.${fieldName} = (object.${fieldName} !== undefined && object.${fieldName} !== null)
                      ? ${readSnippet(`object.${fieldName}`)}
                      : ${fallback};
                `);
        }

Test result

Modified code result in generated constant Information like below

export const Information {
    fromPartial(object: I) {
        const message = createBaseInformation();
        message.name = object.name ?? '';
        message.age = object.age ?? 0;
        message.detail = Information_Detail.fromPartial( object?.location || {} );
    }
}

[jcready]

What happens when the message is recursive?

message Foo {
    Bar bar = 1;
}

message Bar {
    Foo foo = 1;
}

Your proposal would cause a stack overflow.

[stephenh]

@jcready is right that schemas with two-way pointers would infinite loop.

IIRC I've seen the Java protobuf output handles this by creating a static/shared "default foo" / "default bar" instances, but I think only works b/c they are immutable, and any mutations to the default foo/default bar actually create new instances.

Also fwiw @tnhh00037 , personally/for my own codebases, I wouldn't want these defaults applied to the production code paths; imo production code should always fill in real values, and not get for-free defaults.

That said, you mention unit tests, and I think that is more valuable/appropriate use case, basically creating test factories that allow tests to easily get already-filled-in messages.

For test factories, personally I'd want them to be a different method, like Information.createTest or something like that...

Granted, @jcready is right that createTest would either have to: a) avoid cycles somehow, or b) just document that it can only be used in non-cyclic messages, which if it's a new method specifically for tests, is probably fine, until someone who has a cyclic message wants to fix it/support it.

So, I dunno, happy to accept a PR for this, but I think my asks/guidance would be:

• Add a new method, like createTest or propose a better name
• Ideally createTest would call the existing create / fromPartial methods, just with some || defaults applied, like hopefully we don't need to copy/paste the whole generateFromPartial method just to get createTest generated as well

[tnhh00037]

As a former ROS developer ( need to mention beforehand that ROS message doesn't support recursive message ), when defining a ROS message like this

// Information.msg
string name
uint32 age
Detail detail

// Detail.msg
string address

builiding this above message with library like gennodejs will result in Information.js below

class Information {
  constructor(initObject={}) {
        if (initObj === null) {
        this.name = null;
        this.age = null;
        this.detail = null
    }
    else {
      // check for this message's fields by key name - otherwise assign default values
      if (initObj.hasOwnProperty('name')) {
        this.name = initObj.name;
      }
      else {
        this.name = '';
      }

      if (initObj.hasOwnProperty('age')) {
        this.age = initObj.age;
      }
      else {
        this.age = 0;
      }

      if (initObj.hasOwnProperty('detail')) {
        this.detail = initObj.detail;
      }
      else {
        this.detail = new Detail();
      }
    }
  }
}

Because no recursive, so you can create the whole message without worrying about stack overflow. The reason why ROS support this behavior is because it helps ensure consistency and reliability in robotic applications. Without default values, software might encounter runtime errors or unexpected behavior when processing message.

I understand the application between ROS and Protobuf are different. What I want is to extends protobuf's ability so that it can support more use cases, by defining one message interface but can re-use in case Protocol changing from GRPC to something else. Your point of view is a real thing imo production code should always fill in real values, and not get for-free defaults, I totally agree. However

• Default field will be already assigned default value, if it's not nested. Hence, it's not 100% fill in real values.
• Also, "all-are-optional" policy of protobuf might be hurtful in some cases in strict system (if the receiver doesn't use grpc, but expect to receive JSON, then it tends to check all the fields using some kind of schema).

Anyway, let's come back to how we will resolve this. I've got some names: creatWithDefaults, createDefaultObject, setDefaultValues, ... I still aim to use this function in production code, not only in unit test. By using clear-enough name I think user will know when to use what. About the PR, I will think a way to detect whether schema has 2-way pointers. In worst case, I can do like your b's approach, but I think you don't really like this to happen to your product right? :smile:

[stephenh]

Hm. Fwiw I'm not familiar with ROS, but when you mention:

Also, "all-are-optional" policy of protobuf

Doesn't that make this feature less necessary / maybe more hassle than it's worth?

My rationale is that because protobuf messages can never have required children (only scalars can be required), an Information type will always look like:

interface Information {
  detail: Detail | undefined
}

And so client code that does like:

const info = Information.decode(bytes);
console.log(info.detail);

Already has a "a default", which is undefined. So by introducing a "different default", that is used by Information.createWithDefaults, you're console.log actually gets more complicated because it has to handle:

• The "real protobuf default" of info.detail === undefined,
• Your "preferred default" of info.detail === anEmptyDetail,
• A real/populated detail

So you've got three cases to handle now, instead of just two. Which seems like a step backwards.

It seems like to really make this feature useful, you'd want to fundamentally change Information to be:

interface Information {
  detail: Detail
}

So you're code can go back to have "only two" (or even "only one") codepaths: handling info.detail: Detail and not worrying about undefined.

Such a fundamental change would go beyond "a new create method", and really be something fundamentally different like a new option like useDefaultMessages=true, which would:

• Change the type of Information.detail to be "just Detail"
• Not need to add a new createWithDefaults b/c the Information.create would now return the undefined-less types anyway
• Somehow support creating "default messages" (default Details, default Informations) in a way that avoids cycles

So, that's my current/updated thoughts--instead of "a new create method", I think your code/ask is probably better of with this useDefaultMessages option that fundamentally "removes optional-ness / removes undefined" from child message types (because if we don't remove the optional-ness, your client code is going to have to handle the undefined case anyway, so why bother?).

Lmk if that makes sense, and if you'd like to work on a PR that does that. As a disclaimer it will probably be a pretty involved PR.