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.
A common task in module development is locating files or folders in the system filesystem. The current API for doing that requires a rather long line of code which, additionally, exposes users of the API to the concept of a "Repository" which is nearly meaningless in current NetBeans programming, i.e. FileObject theFld = Repository.getDefault().getDefaultFileSystem().findResource ("foo"); I propose adding two methods to FileUtil, getConfigurationFile() and getConfigurationFolder(). Patch with tests attached.
Created attachment 36932 [details] Patch adding two methods to FileUtil
Isn't this a duplicate of an existing RFE? I have a vague recollection there is one like this but I am not sure. getConfigurationFile should check that result.isData(). It should also be made to throw IOException, not catch it. Analogously for getConfigurationFolder but with isFolder of course. I don't like the multiple choices for the arg to getConfigurationFolder. The only correct path for root is "". "/" and null should be illegal. Replace sfs.findResource(path) with root.getFileObject(path). Or just directly call FileUtil.create{Folder,Data}, since this already returns the existing file/folder if there is one. I don't think Repository.getDefaultFileSystem needs to be deprecated. It still works just fine with this patch, and we have no expectation of deprecating the whole class (you may still need to implement it e.g. from unit tests), so why add deprecation warnings to dozens of modules? There also might be some reason why you would want access to the FS instance itself. (Of course you could call FU.gCF("").gFS() as well.) Prefer the test to be named simply FileUtilTest. Test should use MockServices rather than setting the system property. In a test, try { f1 = FileUtil.getConfigurationFolder("other"); } catch (Exception e) { ioe = e; } assertNull(ioe); can be replaced simply with f1 = FileUtil.getConfigurationFolder("other");
Created attachment 39733 [details] Revised patch
Created attachment 39734 [details] Updated patch (containst spec version increment + apichanges updates)
Attached patch addresses most of Jesse's comments. One change: Added a boolean create parameter - sometimes you don't want to create it, so it makes this stuff useful in more situations. - getConfigurationFile should check that result.isData() (same for ...Folder) Assertions added. - "/" and null should be illegal. Done - Replace sfs.findResource(path) with root.getFileObject(path) Done - I don't think Repository.getDefaultFileSystem needs to be deprecated. Removed, note added to its javadoc instead. There are still use cases (like a data loader which should only be active on the SFS) - Prefer the test to be named simply FileUtilTest. Test methods moved to the existing test of that name. - Test should use MockServices Done. - It should also be made to throw IOException, not catch it. With the create argument, the only time it is going to be thrown is if create is true (which I would anticipate being fairly rarely). If the system filesystem is unwritable, something is horribly wrong and likely there are much bigger problems than returning null from here. 99% of the time you're just getting a folder to examine its contents, etc. Seems silly to make every call that wants to fetch a folder from the SFS have to check for IOException - it is logged, and if it happens, much worse things are in store anyway. If we could detect the specific case of a bogus filename, perhaps for that. Every time you make someone catch a checked exception without a very good reason, God kills a kitten.
I vote for documenting it to throw an IOException.
Recheck spec versions: OpenIDE-Module-Specification-Version: 7.2 vs. <version major="7" minor="1"/> and don't forget @since. @return tag needs to be rewritten, it is not accurate. s/file/folder/g in JD for getConfigurationFolder. For testing, it is unnecessary to do Exception x = null; try { something(); } catch (Exception _x) { x = _x; } assertNull(x); You can simply call something(); if it throws an exception the test will of course fail. Typo in apichanges: Respository
If we're determined to have it throw IOException, perhaps the two calls should be split apart - i.e. getConfigurationFile()/getOrCreateConfigurationFile(), etc., with the method that can create the file/folder documented to throw IOException. That way for the simple case that someone just wants to get some folder, they do not need a try/catch block (which they wouldn't need if they called Repository). Any objections to that?
Created attachment 40001 [details] Revised patch separating create/get methods
*** Issue 75035 has been marked as a duplicate of this issue. ***
Any comment on the most recent patch? If not, this should be integratable (now if I could just get commit validation to work properly again...)
[JG01] apichanges.xml refers to Repository but the changes are actually in FileUtil. Please have another look at the Javadoc and behavior of these methods, and make sure you have test coverage for every documented condition, as there are a lot of little mistakes (02-06): [JG02] getConfigurationFile should document that it can return null. [JG03] getConfigurationFile may not throw an assertion error upon encountering a folder at that position. Same for getConfigurationFolder. This is a quite possible situation that must be properly handled. [JG04] getOrCreateConfigurationFile could just call createData immediately, as that already returns any existing file with that path. [JG05] getConfigurationFolder incorrectly talks about creating a nonexistent folder. [JG06] getConfigurationFolder talks about "file" when it should say "folder". Same for getOrCreateConfigurationFolder. [JG07] Repository.getDefaultFileSystem Javadoc should clarify that its contents are up to the repository implementation, though the default implementation present in core/startup behaves as described. [JG08] Repository.getDefaultFileSystem Javadoc should use {@link ...}. [JG09] MyRepo in FileUtilTest is unnecessary. The default Repository uses an in-memory default FS. Clearer and safer to put any setup directly into the tops of the test methods which are actually using it. [JG10] What is FileUtilTest.getResources for? [JG11] Please do not issue gratuitous println's from tests.
I would like to take over this issue and ask reviewers for their review. Please, look at a new patch. It is inspired by Tim's work but it adds just public static FileObject getConfigFileObject(String path)
Created attachment 74911 [details] FileUtil.getConfigFileObject patch.
[JG12] .getRoot().getFileObject(path) can be replaced with .findResource(path). [JG13] "Throws {@code NullPointerException} if the path is {@code null}." is better expressed as @throws NullPointerException if the path if null [JG14] Please replace existing calls to R.gD().gDFS().{gR().gFO(p),fR(p)} with FU.gCFO(p). There seem to be around 570 files using it in main and around 150 in contrib: hg locate -r . -0 \*\*.java | xargs -0 grep -lZ -- \\.getDefaultFileSystem\ \\\?\(\) | xargs -0 ls -l
> getConfigFileObject(String path) [TB1] getConfigFile() or getConfigurationFile() should be sufficient; the return value communicates that the result is a FileObject, the method name doesn't need to specify that [TB2] One of the goals of this RFE was to insulate module authors from the concept of a "Repository" and many cases of interacting with FileSystem. If there are no create* methods, then this goal is not really achieved - if you want to create things in the sfs you still have to know what a "Repository" is. I'd request either restoring those methods, or at the least, FileUtil.getConfigurationFilesystem() or similar.
> [JG12] ...replace with .findResource(path) What is the reason? > [JG13] @throws Will do. > [JG14] replace existing calls... Should we also deprecate the Repository class then? > [TB1] getConfigFile() or getConfigurationFile()... I prefer getConfigFileObject because it is more accurate. > [TB2] ...create* methods I think you don't need to know anything about the Repository. You can just use FileObjects to create what you need. getOrCreate* methods would duplicate existing FileUtil.createFolder/createData. FileObject root = FileUtil.getConfigFileObject(""); FileObject folder = FileUtil.createFolder(root, "a/b/c"); FileObject data = folder.createData("d.txt"); // or FileObject data = FileUtil.createData(root, "a/b/c/d.txt"); // or FileObject data = FileUtil.createData(folder, "d.txt");
Agreed with TB1; the extra "Object" in the name is just noise. But I don't feel strongly about it. To TB2 - using FileUtil.create* methods are probably adequate. Most cases that I know of are read-only operations: you do not need to create the folder if it does not already exist, so just calling FU.gCF and checking for null suffices. For those cases where you do need to write, FU.getConfigFile("") is awkward and not very intuitive. FU.getConfigRoot() might be clearer. Not a major issue. JG12 - just shorter and clearer. Semantically equivalent. JG14 - well, it is still legitimate to subclass it and install a default instance as a service, so I don't think we can deprecate the entire class. We could deprecate getDefault(), which would effectively mean that anyone trying to use it as a _client_ would be reminded to use FU.gCF instead. There might be some code which adds a FCL to the whole config FileSystem; in such a case you would need to use FU.gCF("").getFileSystem(), which is awkward. But I think it is not common. You can simply review the list of current usages (using the command I mentioned earlier) and see for yourself what is commonly needed.
> so I don't think we can deprecate the entire class Wasn't suggesting we deprecate it necessarily, just not have the first thing someone confronts when working with NB filesystems have to say to themselves "WTF is a Repository", and go through the mental process of thinking it's something they need to care about when 99% of the time they'll never use it directly.
> I prefer getConfigFileObject because it is more accurate. If we can avoid verbosity we should - getConfigFile() is briefer, just as clear, and less likely to push someone over the 80 column limit. People learn APIs through code completion, which shows the return type, so the extra precision does not really help anyone, but it does hurt in terms of how verbose the code is. <aside> The real bug is that FileObject was called "FileObject" - same for "DataObject" and "*Object" in general. This is Java. Of course it's an object! Adding "Object" to any class name is pure noise (although there might be some justification of FileObject in that name collisions with java.io.File would be likely and result in verbose code spelling out one FQN or the other; other frameworks get around this by using the term "Resource" although that has its own problem of being too general a term). </aside>
Y01 I think Repository should be deprecated Y02 All usages of Repository.getDefaultFileSystem() shall be removed. Please use http://lang.dev.java.net project: a. include the library and add the @Transformation annotation to Repository methods b. open any source code using the old method and check whether it shows a hint c. use batch apply (from command line) to convert all projects in hg.netbeans.org I'll be in work tomorrow, to help with the setup or ask Jan Lahoda for help.
Again to Y01: we cannot deprecate the whole Repository class, but we could deprecate .getDefault(). Y02a - where would this library go? org-openide-filesystems.jar would need to link against it due to RetentionPolicy.CLASS. I think this requires more careful thought and a separate review. Or do you mean to add Repository.java to nboverlay/src/org/openide/filesystems/ in that project? Y02c - this may work for some usages, but certainly not all. A lot of code uses e.g. Repository r = Repository.getDefault(); FileObject f = r.findResource(...); or FileSystem sfs = Repository.getDefault().getDefaultFileSystem(); FileObject f = sfs.getFileObject(...); or various other idioms. As I understand it, @Pattern will work only on expressions, not sequences of statements. You would need Jackpot or something similarly powerful for that.
> Again to Y01: we cannot deprecate the whole Repository class, but we could deprecate .getDefault(). FileUtil.setMIMEType() is deprecated but valid for use in unit tests; we still deprecate it because use of it in runtime code is wrong. (As one of the few people on the planet who have ever implemented Repository - and that was in a unit test, I ask) are there really any reasonable cases for implementing Repository for use at runtime? Probably it depends on your definition of deprecation, but I'm fine with the "deprecated but if you really know what you're doing use it" variety of deprecation. It's value as advice for what not to do is greater than the annoyance of having to add SuppressWarnings if you really do have a valid use case, given how rare such cases are.
Again, yes there is code that implements Repository (not just written by you) and it is doing nothing wrong. We should never create situations that require legitimate code to use @SuppressWarnings. Please do not deprecate the class; deprecate the getDefault method, which will ensure that no one calls it accidentally, without affecting subclasses.
I'll bow to Jesse's wisdom, although I'm very curious who implements Repository and why.
I count 62 usages in main, mostly in unit tests, of code either extending Repository or just calling its constructor with a default filesystem.
> mostly in unit tests, of code either extending Repository or just calling its constructor with a default filesystem. And a pointer to those non-unit-test cases?
> mostly in unit tests, of code either extending Repository or just calling its constructor with a default filesystem. I worry when I write working code that results in a deprecation warning - that's something I've got to fix. But unit tests? I think there it is the job of the person making a deprecated thing now really broken completely to fix tests that depend on some deprecated thing working. Deprecating something is a hard process that has consequences. I don't think it should be a free ride As you say, > I count 62 usages in main, mostly in unit tests What's the bigger price - that someone justifiably implements Repository isn their unit test, or they do that in their application? Which behavior is more likely to end up with the implementor tied up in knots. Deprecating Repository discourages that wholesale. There may be some case for still implementing it in a test. If there isn't a really good case for implementing it outside of a test, it should be deprecated, even if the test usage is valid.
The usages are in tests except for the standard NbRepository and something in BPEL I don't grok. "making a deprecated thing now really broken completely" - it's not broken at all. The tests are doing nothing wrong. It is perfectly legitimate to create a custom Repository in a unit test. It works now and it has always worked. (Now that the default Repository in a unit test is taken from layers in the classpath - a change in either 6.1 or 6.5, I forget which - some of these tests could delete this trick and still work. But there are other cases where you want to include special items not in layers, exclude some things specified in layers, change contents dynamically, etc.) "If there isn't a really good case for implementing it outside of a test, it should be deprecated, even if the test usage is valid." - again, we _may not_ deprecate code using valid idioms. Test code is as important as regular module code and should be free of warnings. "that someone [...] implements Repository [...] in their application" - no one is accidentally implementing Repository! This is not a serious concern. By contrast, some 700 pieces of code in main & contrib are calling Repository.getDefault() to get access to the standard instance, and probably all of these could and should switch to the proposed method in FileUtil as soon as it is available. BTW it would be even better to deprecate getDefaultFileSystem - it is final, i.e. you can only set the default FS using the super constructor. All we need to ensure is that the class and its constructor are not deprecated. The only caller would then be FileUtil itself, which as a class in the same module would not print a warning. Deprecating just getDefaultFileSystem instead of getDefault would also avoid penalizing legitimate test code which does something like protected @Override void setUp() throws Exception { MockLookup.setInstances(new MyRepo()); assertEquals(MyRepo.class, Repository.getDefault().getClass()); } as a sanity check.
Thank you for comments. I will attach final patch with FileUtil.getConfigFile, getConfigRoot and deprecated Repository.getDefaultFileSystem. I plan to replace as much as possible usages of Repository in our code base.
Created attachment 75124 [details] FileUtil.getConfigFile-2 patch.
Looks good to me. Minor comments: Typo in apichanges: "Respository" "Returns the root of the NetBeans default (system, configuration) filesystems." - final 's' incorrect. Would be helpful to mention in Javadoc of getConfigFile that if you wish to create the file/folder when it does not already exist, start with getConfigRoot and use FileUtil.create* methods.
OK, I will fix typos and javadoc and will integrate the last patch.
Fixed in core-main repository. Replacement in contrib will be done soon. 0ae248c7d963 78e10891a932
Integrated into 'main-golden', will be available in build *200901140201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/0ae248c7d963 User: Jiri Skrivanek <jskrivanek@netbeans.org> Log: #91534 - added FileUtil.getConfigFile(path) and getConfigRoot() to replace Repository.getDefault().getDefaultFileSystem().findResource(path).