Bug 91534

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).