Community
Participate
Working Groups
Created attachment 167766 [details] Removes error logging from MetadataRepositoryElement#fetchChildren MetadataRepositoryElement#fetchChildren contains a catch block for ProvisioningException and reports the exceptions caught to the UI, however the method which throws ProvisioningExceptions also reports them so error messages are duplicated in the UI.
will review for RC1
*** Bug 232436 has been marked as a duplicate of this bug. ***
MetadataRepositoryElement#fetchChildren will not get a ProvisionException because of bug 312308. So double logging is not an issue here. (Perhaps you found this after using the patch bug 312308. This is why I don't want to fix that bug without reexamining all clients, because most clients are logging the exception). In fact, I think it's completely appropriate that if a method throws an exception, the caller should be able to assume that logging is warranted. If it's already been logged by the method throwing it, then why are we throwing it? This should all be sorted out as part of bug 312308. That issue aside, I agree that fetchChildren needs some attention because as is, it will call the super fetchChildren even when the repo load fails (because there is no exception thrown). So the queryable will still be null and it will invoke super to execute the query. However following the code path of QueriedElement#fetchChildren, I see that a QueryDescriptor will get composed, but it will have a null queryable since the load method didn't return a repo. So an empty query result will be returned, and the AvailableIUWrapper will examine results. Since the query result is empty, the getElements() code will end up figuring this out and returning the same "EmptyElementExplanation" (the red X that appears in the list) that would have been returned by MetadataRepositoryElement#fetchChildren had we caught the exception up front. All this is to say that we are taking a very roundabout trip to end up with the same result as if we had caught the exception. I'm marking this bug as dependent on bug 312308. First we need to decide whether ProvisioningUI.loadMetadataRepository is swallowing errors or not. Then decide here what to do based on that decision. I agree it would be best if we noticed the failure immediately and bypassed the superclass query construction.
(In reply to comment #3) > That issue aside, I agree that fetchChildren needs some attention because as > is, it will call the super fetchChildren even when the repo load fails (because > there is no exception thrown). So the queryable will still be null and it will > invoke super to execute the query. > > However following the code path of QueriedElement#fetchChildren, I see that a > QueryDescriptor will get composed, but it will have a null queryable since the > load method didn't return a repo. So an empty query result will be returned, > and the AvailableIUWrapper will examine results. Since the query result is > empty, the getElements() code will end up figuring this out and returning the > same "EmptyElementExplanation" (the red X that appears in the list) that would > have been returned by MetadataRepositoryElement#fetchChildren had we caught the > exception up front. > > All this is to say that we are taking a very roundabout trip to end up with the > same result as if we had caught the exception. I'm not sure if this is what you're saying, but from debugging the super.fetchChildren call results in a second attempt to contact the repository.
removing milestone, I don't see us doing refactoring work here unless we are adding features or working in here otherwise.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
