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 of ElasticSearch access path problem #1099 #1102

Merged
merged 5 commits into from
Jan 13, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

Here is the test which can be used to reproduce the problem in #1099

I will check how to fix it today or tomorrow

@pvojtechovsky pvojtechovsky changed the title test to reproduce ElasticSearch access path problem #1099 fix of ElasticSearch access path problem #1099 Jan 10, 2017
@pvojtechovsky
Copy link
Collaborator Author

There was problem with detection of visibility of nested private classes. Now it correctly decides that all private classes in scope of one top level class can access each other.
This fix slightly changes behavior of some other tests, therefore I updated these existing tests too, so they are passing now.

assertTrue(mmwli.toString().indexOf("AbstractMapBasedMultimap<K, V>.WrappedList.WrappedIterator")>=0);
assertTrue(mm.toString().indexOf("AbstractMapBasedMultimap<K, V>.WrappedList.WrappedIterator")>=0);
assertTrue(mmwli.toString().indexOf("AbstractMapBasedMultimap<K, V>.WrappedCollection.WrappedIterator")>=0);
assertTrue(mm.toString().indexOf("AbstractMapBasedMultimap<K, V>.WrappedCollection.WrappedIterator")>=0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok I think there's a behaviour change here, but maybe it's better, I'll try to clarify it.
What we had before was:

abstract class AbstractMapBasedMultimap<K, V> {
    private class WrappedCollection {
        class WrappedIterator {        }
    }

    private class WrappedList extends AbstractMapBasedMultimap<K, V>.WrappedCollection {
        private class WrappedListIterator extends AbstractMapBasedMultimap<K, V>.WrappedList.WrappedIterator {        }
    }
}

What we have now is:

abstract class AbstractMapBasedMultimap<K, V> {
    private class WrappedCollection {
        class WrappedIterator {        }
    }

    private class WrappedList extends AbstractMapBasedMultimap<K, V>.WrappedCollection {
        private class WrappedListIterator extends AbstractMapBasedMultimap<K, V>.WrappedCollection.WrappedIterator {        }
    }
}

which means if now you redefine class WrappedIterator in WrappedList it won't be take into account.
But the original source code is:

abstract class AbstractMapBasedMultimap<K, V> {
	private class WrappedCollection {
		class WrappedIterator {
		}
	}
	private class WrappedList extends WrappedCollection {
		private class WrappedListIterator extends WrappedIterator {
		}
	}
}

And I don't think the WrappedIterator here point to WrappedList.WrappedIterator... WDYT @tdurieux ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is hard to say what is the best solution.
I cannot find any behavior change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

which means if now you redefine class WrappedIterator in WrappedList it won't be take into account.

We can make a test for that. As long as there is no ambiguous name, the algorithm can use arbitrary access path. But once there are two WrappedIterator instances in different scope, then we will see whether it behaves correctly

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be great to add the test: we check with @tdurieux and it will work as defined in the test. However if you build the model and then you put dynamically a new class WrappedIterator in WrappedList the WrappedListIterator continue to extend the old WrappedIterator and not the newly one.
There is a real discussion here on what should be the behaviour, but I think it's out of the scope of this PR.

surli and others added 2 commits January 13, 2017 11:22
… class. Replace some assertTrue by assertEquals to help debug test.
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.

3 participants