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

ImmutableMappable init fix #1079

Merged
merged 1 commit into from Mar 16, 2020
Merged

Conversation

itruf
Copy link
Contributor

@itruf itruf commented Dec 3, 2019

I've started research to fix warning mentioned here:
#1043 (comment)

If you try to init ImmutableMappable with JSONString - swift shows warning that this method has no calls to throwing functions.

I found that:

  • ImmutableMappable inherits from BaseMappable
  • ImmutableMappable has an extension with init methods which throws (like init(JSONString: String, context: MapContext? = nil) throws)
  • BaseMappable has an extension with optional init methods (like init?(JSONString: String, context: MapContext? = nil))

It causes crashes in some cases.

For example:
let response = try? MyModel(JSONString: jsonString) will crash if MyModel is ImmutableMappable and throws in init method.

I'm not sure but it can be caused by selecting optional init from BaseMappable which not marked with throws.

@itruf
Copy link
Contributor Author

itruf commented Dec 3, 2019

@tristanhimmelman I'm not pretty sure about library architecture. Hope that you will find some time to check my PR.

@itruf itruf changed the title Possible ImmutableMappable fix ImmutableMappable init fix Dec 18, 2019
@itruf
Copy link
Contributor Author

itruf commented Dec 28, 2019

@tristanhimmelman it's a critical problem with ImmutableMappable. Please check it out.

@itruf
Copy link
Contributor Author

itruf commented Mar 1, 2020

@tristanhimmelman I'm really sorry for pushing you but it's the very important PR for the project. I hope that you will take a look at it.

@tristanhimmelman tristanhimmelman merged commit 8b69860 into tristanhimmelman:master Mar 16, 2020
@itruf
Copy link
Contributor Author

itruf commented Mar 16, 2020

Thank you @tristanhimmelman

@murad1981
Copy link

this update caused many crashes in my app where I init an ImmutableMappable from a json string and from a json dict, both now cause runtime crash.

@itruf
Copy link
Contributor Author

itruf commented Mar 16, 2020

@murad1981 can you show an example of your model and a JSON string caused crash? I think that it's correct behavior if you init an ImmutableMappable model with an incorrect JSON.

mbektchiev added a commit to Kinvey/swift-sdk that referenced this pull request Apr 1, 2020
tristanhimmelman/ObjectMapper#1079 has moved
the initializers of `BaseMappable` to `Mappable`. As a workaround we're
defining them once again for `BaseMappable`.
mbektchiev added a commit to Kinvey/swift-sdk that referenced this pull request Apr 2, 2020
tristanhimmelman/ObjectMapper#1079 has moved
the initializers of `BaseMappable` to `Mappable`. As a workaround we're
defining them once again for `BaseMappable`.

refs KDEV-509
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

3 participants