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

Fix request descriptor lookup class matching. #1692

Closed
wants to merge 1 commit into from
Closed

Fix request descriptor lookup class matching. #1692

wants to merge 1 commit into from

Conversation

deanbrowne
Copy link

Class matching was done based on address equality of the Class.
Almost always that is enough, but two different Class structs can be
created to represent the same thing. When this happens RestKit could
not match the registered RequestDescriptor.

This happened to me and I have no way to create a good reproducable
test. I found a [StackOverflow
thread](http://stackoverflow.com/questions/16424298/why-is-class-nsclass
fromstringnsstringfromclass-class-on-os-x) on this happening.

`Class` matching was done based on address equality of the `Class`.
Almost always that is enough, but two different `Class` structs can be
created to represent the same thing.  When this happens RestKit could
not match the registered `RequestDescriptor`.

This happened to me and I have no way to create a good reproducable
test.  I found a [StackOverflow
thread](http://stackoverflow.com/questions/16424298/why-is-class-nsclass
fromstringnsstringfromclass-class-on-os-x) on this happening.
@segiddins
Copy link
Member

@deanbrowne before we think about merging this, could you please add a failing unit test? Thanks

@deanbrowne
Copy link
Author

Could not make one. As I pointed out in the comments it is pretty weird to
get 2 different Class structs. It only happened to me in one of my unit
tests and I was unable to figure out why it worked in my app's code, worked
in most of tests, but no in just one.

On Fri, Nov 29, 2013 at 4:13 PM, segiddins notifications@github.com wrote:

@deanbrowne https://github.com/deanbrowne before we think about merging
this, could you please add a failing unit test? Thanks


Reply to this email directly or view it on GitHubhttps://github.com//pull/1692#issuecomment-29542272
.

@segiddins
Copy link
Member

@deanbrowne without a failing test, I'm hesitant to merge in any changes

@percysnoodle
Copy link
Member

@deanbrowne I had a problem a few weeks ago, not related to RestKit, where I'd accidentally added a .m file from one of my pods to my main target, and so I had two copies of the same class, and it was causing weird problems in places where I was calling isKindOfClass:. Perhaps something similar happened to you with your unit tests?

I'm not sure testing by class name is the best way of checking for this, though; it seems to me that having two classes with the same name but different implementations is just as likely as having two copies of the same class, if not more so.

@percysnoodle
Copy link
Member

@deanbrowne if you want to write a test for multiple classes with the same name, have a look at objc_allocateClassPair and objc_registerClassPair - there's an example of them in use here.

@blakewatters
Copy link
Member

Comparing classes should be sufficient. This probably indicates that you actually have two copies of the class compiled in somehow and are getting different versions randomly. This can happen if you compile one into a static library/testing bundle and another copy into the main app target.

I believe this to be an issue on the user side and do not want to merge any changes.

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