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

Firebase.readEvent() returns "type" instead "put" while getting data from Firebase #374

Merged
merged 16 commits into from Aug 15, 2018

Conversation

kptdobe
Copy link
Contributor

@kptdobe kptdobe commented Aug 8, 2018

PR for #364

Silly memory issue.

Running:

FirebaseObject event = Firebase.readEvent();
String var = event.getString("xyz");
String type = event.getString("type");

will always assign xyz to type even though the type in the JsonVariant is correct at the end of the readEvent method!
I am not a cpp expert but certainly something related with using pointers and memory allocation.
I do not know if this is the correct fix but at least it fixes the streaming issues...

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@kptdobe
Copy link
Contributor Author

kptdobe commented Aug 8, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@kptdobe
Copy link
Contributor Author

kptdobe commented Aug 14, 2018

@proppy Any chance you could review / merge this PR ?

@proppy
Copy link
Collaborator

proppy commented Aug 14, 2018

@kptdobe sorry for the late reply, it seems that this change break the continuous integration (see the red cross near the travis logo).

Maybe using https://arduinojson.org/v5/api/jsonobject/set/ with the arduino String as an argument would be clearer?

@kptdobe
Copy link
Contributor Author

kptdobe commented Aug 15, 2018

I tried multiple versions of the code but no way to make the test pass.

set and = operator are calling the same internal methods which fail. Something is wrong with adding the type property to the JsonObject.

I really do not get why it runs fine with my local Arduino env and it does not in Travis. There is one thing different I cannot identify.

I also tried to update the Travis tests to ArduinoJson v5.13.2 (latest v5 - current runs v5.11.2) but not better.

@kotl
Copy link
Collaborator

kotl commented Aug 15, 2018

Problem with Travis is that test environment uses different String definition and uses std::string underneath:
https://github.com/firebase/firebase-arduino/blob/master/contrib/test/WString.h

@kptdobe
Copy link
Contributor Author

kptdobe commented Aug 15, 2018

@kotl Thanks, this helped me to fix the tests!

Nasty issue. It seems that JsonObject#setdoes not like the std::string from the test environment. Copying the String into a char[] fixes the test issue without breaking the fix I made.

This repairs the Streaming and is test compliant! ;)

@kotl
Copy link
Collaborator

kotl commented Aug 15, 2018

  • Why not use type.c_str() directly?
  • Please revert your changes to the test environment DEFS file -> we want to use ArduinoJson that was specified there before.

@kptdobe
Copy link
Contributor Author

kptdobe commented Aug 15, 2018

  • type.c_str() is the source of the issue and causes the type to always be equal to "type": it returns a pointer while the set method does not copy internally the string and use the pointer as is. And later, the memory at this location does not change anymore, type never get updated anymore. That's why the streaming does not work. I am not a cpp expert, there might be a better explanation though...
  • if you talk about the changes I made to the .travis.yml file, I reverted them already, the PR is only about one file.

// the only supported format for JsonObject#set (it does not like the std::string of the test env)
char *cstr = new char[type.length() + 1];
strcpy(cstr, type.c_str());
obj.getJsonVariant().as<JsonObject&>().set("type", cstr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • needs: delete [] cstr;
  • change strcpy to strncpy

@kotl kotl merged commit 33363f7 into FirebaseExtended:master Aug 15, 2018
@kotl
Copy link
Collaborator

kotl commented Aug 15, 2018

thank you very much for doing this.

// the only supported format for JsonObject#set (it does not like the std::string of the test env)
char *cstr = new char[type.length() + 1];
strncpy(cstr, type.c_str(), type.length() + 1);
obj.getJsonVariant().as<JsonObject&>().set("type", cstr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't that leak?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah missed the delete, I was thinking it might not be safe to delete if JsonVariant keep a ref on it.
But set() documentation seems to imply that it make a copy if you pass a char* https://arduinojson.org/v5/api/jsonobject/set/.

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.

None yet

4 participants