Breadcrumbs not properly treating objects as metadata on android

[the-simian]

Using this line of code:
client.leaveBreadcrumb('foo', {bar: 'baz'});
results in the warning:
Breadcrumb metadata is not a Map or String

here:

  leaveBreadcrumb = (name, metadata) => {
    if (name.constructor !== String) {
      console.warn('Breadcrumb name must be a String');
      return;
    }
    if (metadata == undefined) {
      metadata = {};
    }
    if (metadata.constructor === String) {
      metadata = {'message': metadata };
    }
    if (!(metadata instanceof Map)) {
      console.warn('Breadcrumb metadata is not a Map or String'); //<---------
      return;
    }
    let type = metadata['type'] || 'manual';
    delete metadata['type'];
    NativeClient.leaveBreadcrumb({
      name,
      type,
      metadata: typedMap(metadata)
    });

as a result, the breadcrumbs are never left.

i can see that the incoming metadata is jsut a normal js object. it does not appear to be converted into Map, as-is

[the-simian]

When I allow the object to pass I get
TypeError: expected dynamic type 'string' but had type object.

It seems related to this: #32

[the-simian]

As I'm looking through this code, I see the function here:

/**
 * Convert an object into a structure with types suitable for serializing
 * across to native code.
 */
const typedMap = function(map) {


  const output = {};
  for (const key in map) {
    const value = map[key];
    if (value instanceof Map || value instanceof Object) {
      output[key] = {type: 'map', value: typedMap(value)};
    } else {
      const type = typeof value;
      if (allowedMapObjectTypes.includes(type)) {
        output[key] = {type: type, value: value};
      }
    }
  }
  return output;
};

And i can see it recursively move through the object and turns each object into a serializeable map.

however, the check is for somethign that is an instance of Map, as in either a Map class, or Map prototype. I dont see how anything could ever pass that check, there is no Map constructor in this file (or anywhere), there is only the typedMap function, but it doe not return a type, just JSON

What I am saying is you are checking for instances of classes or prototypes that don't exist.

I have questions:

• What does the Bugsnag Api ultimately expect? (can I used nested objects? Does it have to be a flat object of only strings, etc? The Docs imply that you can do the former, but I can't tell here)
• what should a well formed JSON message look like for metadata? Can you post some clear examples? The only thing the docs say is that the type property is reserved, and must come from the pre-selected list of types. Is that it?
• Where (or what) is Map? (Did you mean 'Object', since this is JS?)

[cooperka]

@the-simian, indeed this should already be fixed by #33. The new version isn't published yet, but you can fork it for now or refer to the commit hash.

Map is a Java type that React Native uses to bridge JS objects. The equivalent would be NSDictionary in ObjectiveC. More info about bridging here.

I'll let a BugSnag owner comment on the rest of your questions, as I don't want to mislead you by getting something wrong in terms of BugSnag's expectations. Seems like they don't have a lot of documentation or unit tests yet.

[the-simian]

Is Map just a globally accessible object therfore? I saw (in the JS code) a check for instanceof Map, but there was no calling code that would have tried to convert the object to Map. Thats why I'm confused, the check was in Js, not Java.

Maybe I'm misunderstanding what's going on. The JSON you pass into the (JS) leaveBreadcrumb method will never be of type Map, because it is just a user defined Object, therefore this method literally never works.

[cooperka]

@the-simian I misunderstood which Map you were talking about. In this case yeah it's a JS Map. You're right, this would never work as intended. It was probably written by someone rusty on their javascript 😛

Feel free to submit a PR and fix this!

[the-simian]

I'd love to submit a Pr, but I was asking questions about what the (at least) Android Interface needs to be. Swapping that out with a plain-old check of an Object ( if (!(metadata instanceof Object)))

or something like:

function isObject (item) {
  return (typeof item === "object" && !Array.isArray(item) && item !== null);
}

if(isObject(metadata))

causes the same error as PR #33 ostensibly fixes. TypeError: expected dynamic type 'string' but had type object.

I also noticed the 'Mapper' function typedMap() alters the structure of the json a bit also... I am appreciating your responses, but I actually needed a little guidance to dig in. I've yet to be able to post a breadcrumb to the API at all. Idk what it really wants to see. eg. Does the incoming object need to be flat? or is nested ok?

[cooperka]

In the latest release v1.1.3, the breadcrumb can only be a string on Android. It can be any plain type on iOS (string, number, boolean, or object consisting of any of these types). PR #33 should have fixed Android, and as of that change you should be able to use at least numbers and booleans. I thought it also worked for objects, but as you point out it may not. With your suggested change above, it should then work for objects too. Does that help?

[the-simian]

Yeah that is helpful, let me make sure I got this right

• you must stringify every value on android
• Android must be flat therefore, as it can only be essentially a 'string dictionary'
• this means anything nested will break android
• this is totally separate from the object/map check.

Do I have the details correct? this is why you wrote the typedMap in the first place? I'll mess around with this and see if I can fix it.

[cooperka]

That's mostly right, but actually nested objects should also work -- they'll just be recursively flattened into a string before logging. But yes, this logic is separate from the object/map check in JS.

[the-simian]

Alright I'll try to fix it, thanks for that answer its exactly what I needed, right now as-is the typedMap does send something broken into android, but I think i know what to do

[cooperka]

Good luck! I'll be happy to review a PR if you end up fixing it.

[kattrali]

Thanks for the debugging, @cooperka, @the-simian - updated this in a patch just now, which will be published shortly.

[the-simian]

@kattrali (cc @cooperka) I'll check it out, but just be aware that even with Object checks like you have in your patch (I already tried that), I was still getting the error:

TypeError: expected dynamic type 'string' but had type object.

The type-checking in the method is easy enough, but I think the Android API is still not handling nested object correctly as-is. I'll see for sure in a short bit, when you publish!

If my suspicion is correct I will open a separate issue for that.

[kattrali]

Ah, thanks for the clarification, @the-simian. I thought this was handled given #33 and 262b991. Let me know if there is a further issue.