Bug 109301
| Summary: | Category.getChildCategories() throws a java.lang.Error | ||
|---|---|---|---|
| Product: | [Retired] Red Hat Web Application Framework | Reporter: | Daniel Berrangé <berrange> |
| Component: | other | Assignee: | Vadim Nasardinov <vnasardinov> |
| Status: | CLOSED RAWHIDE | QA Contact: | Jon Orris <jorris> |
| Severity: | high | Docs Contact: | |
| Priority: | medium | ||
| Version: | nightly | ||
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2006-09-02 17:44:30 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
1. > If we want to leave the existing APIs in there & mark them
> deprected then they should be be made to continue to operate as
> before.
2. > If we want to force people to migrate to new APIs, then we
> should remove the old ones immediately so you get compile
> errors.
I would like to suggest a third possibility:
3. Mark the methods as deprecated AND throw an Error (or
UnsupportedOperationException).
Option 1 doesn't work in practice, because people routinely ignore
deprecation warnings.
Option 2 is painful, because going down that path produces a metric
shitload of compile-time errors leaving you with a very broken system.
Option 3 addresses both of these scenarios neatly. It does not
generate compile-time errors, thus sparing clients a lot of hair
pulling and tooth gnashing. However, it does generate compile-time
warnings for those who care to update their code. Those who don't
care to update will be forced to do so when they run into runtime
errors. An indisputably superior approach any way you look at it.
That said, I caved in to pressure and removed all the 'new
Error("deprecated")' clauses in changes 37767 and 37768.
Urm, Option 3 is what we already had when I entered this ticket & its lame because you never know what code is broken until you click a link and get the error 500. Both Option 2 & 3 result in the same amount of work for the application developer - namely that they have to change their app to use the new APIs. Option 2 is preferable because the compile time errors show exactly where the changes need to be made before you even try to start a server. With option 3, I could fix 9/10 uses of getChildCategories, miss one & not find it till 6 months later when some client happens to execise that piece of code. This is not superior in any way. If I hadn't encountered this bug by accident today we'd have released this 100% broken code for APLAWS without any incling of the problem. If a piece of code is broken & needs updating, we need to know about it & fix it ASAP. cf, the debacle over making setID a no-op & the myriad of subtle bugs we didn't detect till months later. If setID had been deleted we would have found all the problem areas immediately. This bug has been re-introduced in the categorization land. As noted previously if methods are going to be left in place as deprecated, then they should remain fully functional. If not, then they should be deleted altogether, so that we can quickly identify all code that requires fixing. Runtime errors are unacceptable. Fixed in 38168. QA_READY has been deprecated in favor of ON_QA. Please use ON_QA in the future. Moving to ON_QA. Closing old tickets |
Description of problem: A metric tonne of code uses the old java.lang.Collection based APIs in c.a.categorization.Category, which are in the process of being phased out in favour of the new persistence-collection based APIs. This old APIs have been marked deprecated, which is right, but their implementations have been completely removed & replaced with throw java.lang.Error("deprecated"); So, all usage of these methods, while still compiling just fine, throws errors at runtime. Common practice when deprecating methods is to leave the existing impl intact (or provide functionally equiv implemnentation) for some period of time until client code has migrated to new APIs. If we want to leave the existing APIs in there & mark them deprected then they should be be made to continue to operate as before. If we want to force people to migrate to new APIs, then we should remove the old ones immediately so you get compile errors. Version-Release number of selected component (if applicable): How reproducible: Steps to Reproduce: 1. 2. 3. Actual results: Expected results: Additional info: