This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Looking once again at the Places API (created as fix for bug 57798), I believe it offers its users somethingthat they are supposed to never use. The getters are OK, but the setters are only for privileged users (e.g. core.startup) or for unit tests. Nobody else should ever use them. Thus I am reporting this like-to-be TCR. I can see two ways to do it. Either rely on System properties (as has been the case so far), which is in my opinion OK in tests. Or Lookup.getDefault().lookup(Places.class) implemented in core.startup with additional protected abstract findCacheDir() & findUserDir() to call from the getters.
(Jesse's reply to previous comment) > Nobody else should ever use them. So, do not use them. Is there any evidence that anyone would "accidentally" call these methods? Existing code could already call System.setProperty("netbeans.user", ...) but that was never a threat. A setter with Javadoc explaining its intended scope of usage (and friendly to JPDA breakpoints, logging, Find Usages, etc.) is if anything safer than a system property. (By the way I initially prototyped throwing SecurityException when an attempt was made to change the value once set. This did not work, as usages from TestCase.setUp need to modify an existing value in the same JVM.) > Lookup.getDefault().lookup(Places.class) implemented in core.startup Possible, though this would be more cumbersome for tests; see several test usages in the patch. > I could/can create new bug, if desired. That would be clearer.
(In reply to comment #1) > (Jesse's reply to previous comment) > > Nobody else should ever use them. > > System.setProperty("netbeans.user", ...) but that was never a threat. It was not, as the value was only read at beggining and changing it later had no impact on the system. I think this is acceptable for a property, but having a setter which basically says: don't call me, as it is too late anyway, is absurd. > A setter with Javadoc explaining its intended scope of usage Are you saying that for the lack of better place to put javadoc to, you decided to create a setter!? I don't think it is good justification of its existence. > > Lookup.getDefault().lookup(Places.class) implemented in core.startup > > Possible, though this would be more cumbersome for tests; see several test > usages in the patch. The default implementation of Places would continue to read the property. So we would just return setters in tests back to their state before Places.setXYZ was created. > (By the way I initially prototyped throwing SecurityException when an attempt > was made to change the value once set. This did not work, as usages from > TestCase.setUp need to modify an existing value in the same JVM.) I see.
Created attachment 110238 [details] Variant with subclassable Places
If reviewers don't mind, this is the tweak I'd like to apply to bug 57798 to remove setters from the Places API. One note: looks like support for "memory" does not need to be in openide.modules, just in core.startup...
(In reply to comment #2) >> System.setProperty("netbeans.user", ...) ...was never a threat. > > ...the value was only read at beginning and changing it later had > no impact on the system. Not true. Lots of modules were calling System.getProperty("netbeans.user"). > a setter which basically says: don't call me, as it is too late anyway That is not what it says. It just says that it will affect the return value of the next call to the getter. Obviously the parts of core which interpret the user dir have mostly decided what they will use (config/ location, etc.) during startup. But other code which looks for the user dir at runtime just uses the current value, and it is for this reason that calling the setter from setUp in a unit test is helpful - even if this is not the first test case in the suite. > Are you saying that for the lack of better place to put Javadoc, you decided > to create a setter? No, it is a side benefit. > we would just return setters in tests back to their state before > Places.setXYZ was created. Possible, but this seems like a regression in clarity and safety. -0 on such a change.
(In reply to comment #5) > (In reply to comment #2) > >> System.setProperty("netbeans.user", ...) ...was never a threat. > > > > ...the value was only read at beginning and changing it later had > > no impact on the system. > > Not true. Lots of modules were calling System.getProperty("netbeans.user"). A lot of modules can call it, but if the value is different than the one obtained by SystemFileSystem, everything will be broken anyway. There is only one piece of code who can use the setters in production system: the runtime container. > current value, and it is for this reason that calling the setter from setUp in > a unit test is helpful - even if this is not the first test case in the suite. > > we would just return setters in tests back to their state before > > Places.setXYZ was created. > > Possible, but this seems like a regression in clarity and safety. -0 on such a > change. You were trying to design an API for tests and mistakenly put it into product API! I see. Feel free to design openide.modules/test/unit/src/**/PlacesUtils and put the setters there to improve clarity of test code.
(In reply to comment #6) >> Lots of modules were calling System.getProperty("netbeans.user"). > > A lot of modules can call it, but if the value is different than the one > obtained by SystemFileSystem, everything will be broken anyway. No, not necessarily. Most callers are looking for something unrelated to the config/ subdir. In fact there is not much excuse to use the system property to find the config/ subdir, since it is better manipulated via the FS API.
(In reply to comment #7) > (In reply to comment #6) > >> Lots of modules were calling System.getProperty("netbeans.user"). > > > > A lot of modules can call it, but if the value is different than the one > > obtained by SystemFileSystem, everything will be broken anyway. > > No, not necessarily. Most callers are looking for something unrelated to the > config/ subdir. In fact there is not much excuse to use the system property to > find the config/ subdir, since it is better manipulated via the FS API. The "netbeans.user" is a "read-only" property. Once Repository.getDefault() is created, it is insane to try to change the property in production system. If somebody changed the value of the property later, there would be inconsistency between location of settings directory and "netbeans.user". A clear victim of such inconsistency would be autoupdate.services and updater.jar. I am sure this is not the only case.
(In reply to comment #8) > The "netbeans.user" is a "read-only" property. Once Repository.getDefault() is > created, it is insane to try to change the property in production system. Obviously. And no one _was_ trying to do so.
(In reply to comment #9) > Obviously. And no one _was_ trying to do so. Right and that is the reason why we should not encourage people to think about such crazy actions by exposing setter that should not be called (except from early bootstrap code and tests). Unless there are new objections I'd like to integreate my fix tomorrow.
(In reply to comment #10) > Right and that is the reason why we should not encourage people to think about > such crazy actions by exposing setter that should not be called (except from > early bootstrap code and tests). Yes and no. If you can avoid it and there is a simple way to do it they yes sure. But sometimes for the clarity and simplicity of our code base a method in API which purpose is only to allow or support unit testing is perfectly OK. Unit testing is important part of any software product and so if needed the product source code should have a hooks to make testing easier. I would argue that that is the best practice. Looking at your diff I much more prefer Places.setUserDirectory than dealing with some property. API defensiveness is the goal but we should not be so pedantic about it that we actually harming ourselves and making things more complicated than they should be. People can read and if you say in Javadoc that purpose of a method is to be called only from unit test then that's what they will do. And just because there a few API abusers type of criminals we should not discriminate majority of us. Sometimes naming method appropriately can help as well, for example Places.setUserDirectoryFromUnitTest. In this case I'm in favor of keeping setters. I would probably be more explicit in Javadoc and say the only allowed callers of this method (apart from NB startup routine) are unit tests.
(In reply to comment #11) > (In reply to comment #10) > Sometimes naming method appropriately can help as well, for example > Places.setUserDirectoryFromUnitTest. I am not sure what is priority of your comment. Whether TCR or TCA. If TCR I will create openide.modules/test/unit/src/org/openide/modules/api/PlacesTestUtils.setUserDirectory(...) and use it from the tests that my patch touches.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > Sometimes naming method appropriately can help as well, for example > > Places.setUserDirectoryFromUnitTest. > > I am not sure what is priority of your comment. I'm questioning the purpose of whole this issue. Instead of simple class with two getters and setters you are proposing to turn it into abstract class which needs to be overridden, needs to involve lookup and will introduce special testing support in form of PlacesTestUtils.setUserDirectory. And why do you want to do all that? Because of API design paranoia that somebody might abuse it. Is all this added complexity really worth it? I do not think so. My two cents.
> > I am not sure what is priority of your comment. Omition to specify priority has been interpreted as no objection: ergonomics#ee2e86ab090b > API design paranoia that somebody might abuse it That is how API design works and how we do it in NetBeans. > Instead of simple class with two setters I promised to not reference myself once, but this is not reference to my text, but rather something that Jesse originally wrote. Page 74 of the API book: "Do not put setters where they don't belong". I am glad Jesse is consistent and does not complain when I am trying to follow his advice.
(In reply to comment #14) > Jesse originally wrote [...] "Do not put setters where they don't belong" This was about something entirely unrelated - that setEnabled does not belong in the Action interface but rather in a protected method in AbstractAction (if at all), since it is meaningful only for a small class of actions, whereas in most cases "enablement" is not an externally configurable property but rather a constant (true) or a derivative of some other information (like the selection). > I am glad Jesse [...] does not complain For the record I still feel this change is a bit for the worse, it is just not important enough to spend more time debating.
(In reply to comment #15) > (In reply to comment #14) > > Jesse originally wrote [...] "Do not put setters where they don't belong" > > that setEnabled does not belong > in the Action interface but rather in a protected method in AbstractAction (if > at all) Right. That is the same with Places. Setters belong into subclasses (NbPlaces) or test support methods (if at all). > since it is meaningful only for a small class of actions, Since setters are meaningful only to core.startup (and possibly few tests). > whereas in > most cases "enablement" is not an externally configurable property but rather a > constant (true) Because in most cases "netbeans.user" is not externally configurable property, but rather a constant value, which can only be read and should never be modified.
(In reply to comment #16) > Setters belong into subclasses (NbPlaces) Making Places into an abstract class was your invention, apparently to justify removing the setters. In the original API change it was merely a language-friendly and documentable singleton encapsulation of what had previously been system properties, so there was only one "case" and it needed setters.
(In reply to comment #14) > > > I am not sure what is priority of your comment. > > Omition to specify priority has been interpreted as no objection: > ergonomics#ee2e86ab090b Watch out Jarda!! You are turning into a bureaucratic bot! :-) > > API design paranoia that somebody might abuse it > > That is how API design works and how we do it in NetBeans. Following a dogma is wrong. Especially in case of software practice which is immature and keeps evolving as we speak. We, Netbeans team, do outstanding job when it comes to API design, reviews and maintaining backward compatibility. You realize that when you get exposed to other projects producing an API. And that in part, Jarda, is due to your passion for API. Everybody knows what a pain you can be when it comes to API. :-) So thanks for that. I respect that. But please do not close yourself from compromises. 100% perfect API is a holy grail which we should not aim for. Simplicity of code, it's readability, maintainability is equally important.
Integrated into 'main-golden' Changeset: http://hg.netbeans.org/main-golden/rev/ee2e86ab090b User: Jaroslav Tulach <jtulach@netbeans.org> Log: #201350: Setters replaced by injectable singleton in production code. Utility class provided for unit tests.