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

Unable to initialise a null map field by field number #373

Closed
dave opened this issue May 27, 2020 · 4 comments · Fixed by #707
Closed

Unable to initialise a null map field by field number #373

dave opened this issue May 27, 2020 · 4 comments · Fixed by #707

Comments

@dave
Copy link

dave commented May 27, 2020

If a map field isn't set (e.g. it's null), then it doesn't seem to be possible to modify it by field number. It looks like getField returns the default value, and updates to that value are not reflected in the message... I've prepared a simplified example:

message Holder {
  map<string, int32> numbers = 1;
}
setMapByField(GeneratedMessage message, int field, dynamic key, dynamic value) {
  var map = message.getField(field) as PbMap;
  map[key] = value;
}

void main() {
  // this test passes:
  test("empty map", () {
    var holder = Holder()..numbers.clear(); // holder.numbers map is initialised but empty
    setMapByField(holder, 1, "a", 1);
    expect(holder.numbers["a"], 1);
  });

  // this test fails:
  test("null map", () {
    var holder = Holder(); // holder.numbers map is not initialised
    setMapByField(holder, 1, "a", 1);
    expect(holder.numbers["a"], 1);
  });
}

Is there any way around this?

dave added a commit to dave/protobuf that referenced this issue May 28, 2020
Before this change, repeated fields are initialised in the _getDefault
method but maps are not. This change initialises maps.

This fixes google#373
@dave
Copy link
Author

dave commented May 28, 2020

I believe this can be fixed with a change to the _getDefault method... This method already initialises repeated fields, and can be simply extended to also initialise maps.

I've sent a quick PR showing a potential fix. I'm not totally familiar with the internals so it's possible I've missed something, but this fixes my bug and the protobuf tests all pass. Let me know what you think.

dave added a commit to dave/protobuf that referenced this issue Jul 24, 2020
Before this change, repeated fields are initialised in the _getDefault
method but maps are not. This change initialises maps.

This fixes google#373
dave added a commit to dave/protobuf that referenced this issue Nov 2, 2020
Before this change, repeated fields are initialised in the _getDefault
method but maps are not. This change initialises maps.

This fixes google#373
@nwparker
Copy link
Contributor

nwparker commented Apr 7, 2021

@sigurdm - Do you mind taking a look at this?

This is going back awhile, but it's related to this: #340 (comment)

In other words, your suggestion there won't work for the reason @dave has pointed out

@tp
Copy link

tp commented Apr 17, 2021

Would be awesome to get dave@f0605ba in, as I also ran into this issue, and there doesn't seem to the a "client side" workaround (apart from switching to Dave's branch).

@osa1
Copy link
Member

osa1 commented Apr 5, 2022

Unfortunately dave@f0605ba is not a good fix for this issue, and it breaks a few things. I'm currently working on fixing this. In the meantime here's a hacky workaround:

Proto file:

syntax = "proto3";

message A {
  map<int32, int32> a = 1;
}

Dart:

var a = A.create();

// Get index of the map field in generated message. 1 here is the tag of
// the field as specified in the proto definition.
var mapFieldInfo = a.info_.fieldInfo[1]!;
var mapFieldIndex = mapFieldInfo.index!;

PbMap<int, int> map = a.$_getMap(mapFieldIndex) as PbMap<int, int>;

map[1] = 2;
print(a); // prints "a : {1 : 2}"

@osa1 osa1 closed this as completed in #707 Jul 11, 2022
osa1 added a commit that referenced this issue Jul 11, 2022
- `$_getMap` when called on an uninitialized map field initializes the field.
  Update `getField` to do the same.

  `getField` already initializes repeated fields, but it used to not initialize
  map fields.

  Fixes #373

  This is a sync of cl/442686654.

- Inline `_getDefaultList` into `_$getList` and `_getDefaultMap` into
  `_$getMap`. These inlined functions have only one use site, and inlining them
  makes the `_$getList` and `_$getMap` easier to compare for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants