Bug 198060 - SharabilityQuery / SharabilityQueryImplementation and CollocationQuery / CollocationQueryImplementation should be extended to use FileObject or URI
SharabilityQuery / SharabilityQueryImplementation and CollocationQuery / Coll...
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Infrastructure
7.0
All All
: P3 (vote)
: 7.2
Assigned To: Alexander Simon
issues@projects
CNDREQ_POSTPONED
: API, API_REVIEW_FAST
Depends on:
Blocks: 195124 206932 207288
  Show dependency treegraph
 
Reported: 2011-04-25 13:58 UTC by Vladimir Kvashin
Modified: 2012-01-13 21:51 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
initial proposal (7.30 KB, patch)
2011-12-19 16:51 UTC, Alexander Simon
Details | Diff
second proposal (fixed JG01-JG05, Y01) (44.72 KB, patch)
2011-12-20 11:58 UTC, Alexander Simon
Details | Diff
second proposal (fixed JG01-JG05, Y01) + check scheme (45.77 KB, patch)
2011-12-20 14:12 UTC, Alexander Simon
Details | Diff
+VK01 (47.97 KB, patch)
2011-12-21 06:22 UTC, Alexander Simon
Details | Diff
variant #5 (fixed JG04, JG06-JG13) (54.49 KB, patch)
2011-12-22 16:27 UTC, Alexander Simon
Details | Diff
variant #6 (fixed JG15) (62.50 KB, patch)
2011-12-23 10:24 UTC, Alexander Simon
Details | Diff
variant #7 (fixed JG16-JG22) (62.62 KB, patch)
2012-01-10 08:36 UTC, Alexander Simon
Details | Diff
variant #7 (fixed JG16-JG22) (64.41 KB, patch)
2012-01-10 08:51 UTC, Alexander Simon
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir Kvashin 2011-04-25 13:58:38 UTC
Now SharabilityQuery and SharabilityQueryImplementation signatures use java.io.File. This disallows use of versioning on non-file-based file systems (e.g. remote file system).
Comment 1 Vladimir Kvashin 2011-04-25 13:59:18 UTC
The proposal is:

1) extend SharabilityQueryImplementation SPI with a method
    int getSharability(FileObject fileObj);

2) extends SharabilityQuery API with a method
    public static int getSharability(FileObject fileObj)
Comment 2 Tomas Stupka 2011-04-26 10:54:13 UTC
this shouldn't be solved in VCS
Comment 3 Jesse Glick 2011-04-28 07:45:58 UTC
Would probably use URL rather than FileObject.

Only useful in the context of a FileObject-based VCS, of which there is no existing or planned implementation.
Comment 4 Vladimir Voskresensky 2011-12-16 10:50:02 UTC
FileObject to URL and vice versa is rather expensive conversion to be used in queries.
I think we can do the following:
Introduce FileObject based SPI/API pair in queries.
versioning.core defines API call in terms of VCSFileProxy,
masterfs/remotefs versioning bridges will redirect query from FileObject to VCSFileProxy
Comment 5 Tomas Stupka 2011-12-16 16:56:21 UTC
(In reply to comment #4)
> FileObject to URL and vice versa is rather expensive conversion to be used in
> queries.
> I think we can do the following:
> Introduce FileObject based SPI/API pair in queries.
no strong opinion about URL vs FileObject, but regarding FileObject we have to be careful when handling filesystem events 

calls on sharability from versioning:
a) in versioning.core - when creating a VCSContext for menu actions to compute relevant (sharable) files from nodes. In this case we already operate with FileObjects as well. 

b) in particular vcs modules - to find out if a file is sharable (to be ignored) or not. For several reasons we mind doing status computation synchronously to filesystem events so should be no problem to obtain a FileObject from a file/VCSProxy/remote FileObject.

> versioning.core defines API call in terms of VCSFileProxy,
> masterfs/remotefs versioning bridges will redirect query from FileObject to
> VCSFileProxy
regarding from where vcs modules should access sharability and besides that this is probably a bit out of scope:

because of b) we would have to route the sharability call from vcs modules through versioning to filesystem bridges - what would be the gain in it? sooner or later VCSFileProxy has to be converted to FileObject anyway so why not doing it directly in the vcs module? Would have to make sure it doesn't happen synchronously to filesystem, but afaik we made sure in the existing vcs modules that status computation, (including sharability) isn't called in such scenarios.
Comment 6 Vladimir Voskresensky 2011-12-16 20:27:51 UTC
(In reply to comment #5)
I thought you wanted versioning to be completely FileObject agnostic, but if it is OK just to introduce FileObject-based version of SharabilityQueryImplementation then it would simplify things a lot
Comment 7 Tomas Stupka 2011-12-16 21:46:21 UTC
(In reply to comment #6)
> (In reply to comment #5)
> I thought you wanted versioning to be completely FileObject agnostic, 
already mentioned at some other place that it deals also with FileObject-s, its just that there are limitations when e.g. handling filesystem events.

> but if it
> is OK just to introduce FileObject-based version of
> SharabilityQueryImplementation then it would simplify things a lot
on the other hand the versioning systems have to be quite careful then when converting to FileObject 

using URL probably would be closer to how SharabilityQuery is designed today

also curious what jarda and jesse think about it
Comment 8 Jesse Glick 2011-12-16 22:01:22 UTC
URL makes the most sense to me; SharabilityQuery is able to report whether something should be versioned before you actually create, which would be impossible if using FileObject. SQ is not typically called very often (that I know of), so the overhead of FileObject <-> URL conversion does not seem like an issue; and URLMapper could probably also cache recent queries.
Comment 9 Tomas Stupka 2011-12-16 22:11:19 UTC
> SQ is not typically called very often (that I
> know of), 
from versioning typically not very often. The only thing i can think of at the moment is when suddenly status has to be determined for many yet unknown new files, but that would anyway happen in the scope of a status command which already takes some time.
Comment 10 Vladimir Voskresensky 2011-12-19 08:22:58 UTC
I've checked SharabilityQuery.getSharability(java.io.File) usages in nb repository and there is no work on not existing elements:
1) project implementations, o.openidex.util, refactoring and even core versioning [1] always do FO->File conversion like:
File f = FileUtil.toFile(file);
if (f != null) {
 SharabilityQuery.getSharability(f);
} 

2) particual VCS impls call method in isIgnored implementations which should be extended to use VCSFileProxy anyway 
as mentioned above: versioning core itself uses FO->File conversion.

[1] always in src and in the most cases in tests as well
groovy.grailsproject/src/org/netbeans/modules/groovy/grailsproject/GrailsSources.java
project.ant/src/org/netbeans/spi/project/support/ant/SourcesHelper.java
projectuiapi/src/org/netbeans/modules/project/uiapi/DefaultProjectOperationsImplementation.java
ant.freeform/test/unit/src/org/netbeans/modules/ant/freeform/FreeformSharabilityQueryTest.java
java.j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/J2SESharabilityQueryTest.java
maven/src/org/netbeans/modules/maven/classpath/MavenSourcesImpl.java
apisupport.wizards/src/org/netbeans/modules/apisupport/project/ui/wizard/project/NewProjectIterator.java
projectapi/src/org/netbeans/spi/project/support/GenericSources.java
projectui/src/org/netbeans/modules/project/ui/groups/DirectoryGroup.java
projectuiapi/src/org/netbeans/modules/project/uiapi/DefaultProjectOperationsImplementation.java
refactoring.api/src/org/netbeans/modules/refactoring/api/AbstractRefactoring.java
o.openidex.util/src/org/openidex/search/SharabilityFilter.java
web.project/test/unit/src/org/netbeans/modules/web/project/WebSharabilityQueryTest.java
versioning/src/org/netbeans/modules/versioning/spi/VCSContext.java
Comment 11 Alexander Simon 2011-12-19 16:51:57 UTC
Created attachment 114310 [details]
initial proposal
Comment 12 Jesse Glick 2011-12-19 18:44:45 UTC
Procedural: http://wiki.netbeans.org/Review_Steps#Step_2_-_Submit_the_Issue


[JG01] URI is also an option, to match FileOwnerQuery; no strong opinion regarding URI vs. URL in this case, but URI is generally easier to interconvert with File and is easier to work with.


[JG02] Prefer to introduce fresh interface SharabilityQueryImplementation2, not inherit from old one, so we can deprecate the old one; and deprecate getSharability(File) but make it delegate to the new interface where possible.


[JG03] Beware that URI.normalize is different from FileUtil.normalizeFile. The former is platform-independent and merely checks syntax: no ../ sequences, etc. The latter is platform-specific and additionally checks case folding on Windows and Mac, DOS-style 8.3 filenames on Windows, etc.


[JG04] Should provide an SQI2 impl in projectapi like ProjectSharabilityQuery.


[JG05] Since the API is being touched anyway, this might be a good time to replace the int constants in SQ with an enum. Of course the existing gS(File) would continue to use the old constants, but these could be deprecated for new code.
Comment 13 Jaroslav Tulach 2011-12-19 20:42:20 UTC
Y01 Tests. Especially for bridging between API and SharebilityImplementation and SharebilityImplementation2.
Comment 14 Vladimir Kvashin 2011-12-20 10:23:26 UTC
[VK01] I think that SharabilityQuery should have method
    getSharability(FileObject)
(in addition to URL or URI based),
this will simplify clients who already know file objects
Comment 15 Alexander Simon 2011-12-20 11:58:49 UTC
Created attachment 114349 [details]
second proposal (fixed JG01-JG05, Y01)
Comment 16 Alexander Simon 2011-12-20 14:12:46 UTC
Created attachment 114357 [details]
second proposal (fixed JG01-JG05, Y01) + check scheme
Comment 17 Vladimir Voskresensky 2011-12-20 14:42:14 UTC
I'm OK to have SPI to be URI based.
but what about VK01? 
This is the most convenient API call needed for at least 15 places and it will simplify move of other projects to be full-remote capable.
Comment 18 Alexander Simon 2011-12-20 15:06:56 UTC
(In reply to comment #17)
> I'm OK to have SPI to be URI based.
> but what about VK01? 
> This is the most convenient API call needed for at least 15 places and it will
> simplify move of other projects to be full-remote capable.
Why API should have duplicated methods?
Clients can convert FO to URI by:
    FileObject.getURL().toURI();
(and correctly process exceptions)
Probably you are talking about new method:
    FileObject.getURI();
or
    FileUtil.toURI(FileObject)
that help to convert FO->URI.
Comment 19 Vladimir Voskresensky 2011-12-20 15:28:53 UTC
(In reply to comment #18)
> (In reply to comment #17)
> > I'm OK to have SPI to be URI based.
> > but what about VK01? 
> > This is the most convenient API call needed for at least 15 places and it will
> > simplify move of other projects to be full-remote capable.
> Why API should have duplicated methods?
> Clients can convert FO to URI by:
>     FileObject.getURL().toURI();
> (and correctly process exceptions)
> Probably you are talking about new method:
>     FileObject.getURI();
> or
>     FileUtil.toURI(FileObject)
> that help to convert FO->URI.
No, I'm talking exactly about api user friendly method 
Sharability SharabilityQuery.getSharability(FileObject)
Could you, please, add it.
Thanks.
Comment 20 Alexander Simon 2011-12-20 16:03:21 UTC
(In reply to comment #19)
> No, I'm talking exactly about api user friendly method 
> Sharability SharabilityQuery.getSharability(FileObject)
> Could you, please, add it.
> Thanks.
Jesse, Tomas, Jaroslav do you agree?
Comment 21 Tomas Stupka 2011-12-20 16:27:50 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > No, I'm talking exactly about api user friendly method 
> > Sharability SharabilityQuery.getSharability(FileObject)
> > Could you, please, add it.
> > Thanks.
> Jesse, Tomas, Jaroslav do you agree?
no strong opinion
Comment 22 Alexander Simon 2011-12-21 06:22:00 UTC
Created attachment 114382 [details]
+VK01
Comment 23 Jesse Glick 2011-12-22 13:41:00 UTC
VK01 seems fine as a convenience in the API, so long as the SPI is based on URI. (If and when there is some performance issue that mandates direct API-to-SPI communication using FileObject, a FileObject-based SPI could be introduced; but I expect such issues could be fixed by local optimizations such as caching FileObject <-> URI interconversions.)


JG04 does not appear to have been addressed.


[JG06] It seems you are also touching CollocationQuery. Probably fine, but should be announced in the issue title etc. (which is already obsolete since the SPI uses URI not FileObject). I expect that an apichanges.xml patch will be included in a forthcoming revision.


[JG07] @Deprecated should be paired with a Javadoc tag @deprecated which {@link ...}s to the recommended variant.


[JG08] Reopening JG03 for CollocationQuery.areCollocated(File,File) and .findRoot(File). Check FileUtil.normalizeFile on the arguments if necessary.

(And why would URIs not be normalized on Macs?)


[JG09] Missing Javadoc on Sharability.

Also note that nested enums are implicitly static.


[JG10] FileObject.getURL may not return null, so there is no need to check for this case.


[JG11] (@link ...) should be {@link ...}


[JG12] CollocationQueryImplementation2 missing @since


[JG13] @return {@link org.netbeans.api.queries.SharabilityQuery.Sharability} is pointless since the Javadoc tool always hyperlinks the return type. Just give a brief description, e.g. "@return recommendation"


[JG14] It would be good to add methods in AntProjectHelper to return the new (non-deprecated) SPIs, deprecating the old factories.


[JG15] Changing known usages of proposed @Deprecated members (incl. any from JG14), other than ProjectSharabilityQuery, is recommended as part of a proposed patch. (It is more efficient to do such changes in bulk, and it ensures that your proposal does not harm anything.) Introducing a Jackpot refactoring (META-INF/upgrade/*.hint) makes this easier and helps external module developers too.
Comment 24 Alexander Simon 2011-12-22 15:02:21 UTC
(In reply to comment #23)
> [JG10] FileObject.getURL may not return null, so there is no need to check for
> this case.
Function URLMapper.findURL(FileObject, int) does not guarantee not null result.
Comment 25 Jesse Glick 2011-12-22 15:13:34 UTC
(In reply to comment #24)
>> FileObject.getURL may not return null
>
> Function URLMapper.findURL(FileObject, int) does not guarantee not null result.

It does if you pass INTERNAL, as FileObject.getURL does. (Technically it could still return null in case FileStateInvalidException were thrown from FileObject.getFileSystem(), but in fact no implementations of this method do so.)

At any rate this is not your problem because FileObject.getURL is not documented to return null under any circumstances; if it ever did, that would just be a bug in filesystems.
Comment 26 Alexander Simon 2011-12-22 15:21:03 UTC
(In reply to comment #23)
> [JG14] It would be good to add methods in AntProjectHelper to return the new
> (non-deprecated) SPIs, deprecating the old factories.
Existing non-File based project (MakeProject) cannot reuse AntProjectHelper.
IMHO AntProjectHelper at first should be changed for reusing in non-File based
project. But it is out of scope of this issue.
Comment 27 Alexander Simon 2011-12-22 15:35:56 UTC
(In reply to comment #23)
> [JG15] Changing known usages of proposed @Deprecated members (incl. any from
> JG14), other than ProjectSharabilityQuery, is recommended as part of a proposed
> patch. (It is more efficient to do such changes in bulk, and it ensures that
> your proposal does not harm anything.) Introducing a Jackpot refactoring
> (META-INF/upgrade/*.hint) makes this easier and helps external module
> developers too.
It seems usages of SharabilityQuery cannot be automatically converted. For example, method GenericSources.Group.contains(FileObject):

  File f = FileUtil.toFile(file);
  if (f != null) {
     // MIXED, UNKNOWN, and SHARABLE -> include it
     return SharabilityQuery.getSharability(f) != SharabilityQuery.NOT_SHARABLE;
  } else {
     // Not on disk, include it.
     return true;
  }

has a comment that prevent obvious conversion to:

  return SharabilityQuery.getSharability(file)  != SharabilityQuery.NOT_SHARABLE;
Comment 28 Jesse Glick 2011-12-22 16:14:50 UTC
(In reply to comment #26)
>> [JG14] add methods in AntProjectHelper to return the new (non-deprecated) SPIs
>
> Existing non-File based project (MakeProject) cannot reuse AntProjectHelper.

Regardless, official APIs like AntProjectHelper should be made to refer to the new SPIs so that all usages of the old deprecated SPIs (other than compatibility code like SharabilityQuery itself, and proxies like ProjectSharabilityQuery) can be removed.


(In reply to comment #27)
> It seems usages of SharabilityQuery cannot be automatically converted.

If some cannot, that is fine. At least common patterns like SharabilityQuery.getSharability(FileUtil.toFile(fo)) should be convertible by script. Other cases will just require human review.

> GenericSources.Group.contains(FileObject):
>   if (f != null) {
>      ...
>   } else {
>      // Not on disk, include it.
>      return true;
>   }

In this case the method just had to return _something_ if passed a remote FileObject, yet the API did not offer information about it, so it returned a sensible default. Probably this can now be rewritten to the simpler and more general:

return SharabilityQuery.getSharability(file) != SharabilityQuery.Sharability.NOT_SHARABLE;
Comment 29 Alexander Simon 2011-12-22 16:27:39 UTC
Created attachment 114412 [details]
variant #5 (fixed JG04, JG06-JG13)
Comment 30 Alexander Simon 2011-12-23 10:24:28 UTC
Created attachment 114424 [details]
variant #6 (fixed JG15)
Comment 31 Vladimir Voskresensky 2011-12-29 13:58:09 UTC
If there are no more questions, let's integrate tomorrow
Comment 32 Jesse Glick 2011-12-29 14:26:33 UTC
[JG16] The diff to projectapi/apichanges.xml is unnecessary, and in fact is likely to break the Javadoc build since it refers to a class in a nonpublic package.


[JG17] <date> in apichanges.xml should match actual date of integration to trunk.


[JG18] Typos: "Depricated", "pissible"


[JG19] {@link org.netbeans.api.queries.CollocationQuery#areCollocated(java.net.URI, java.net.URI)} can be simplified to {@link #areCollocated(URI, URI)}; similarly for findRoot, and some links in SharabilityQuery. (Not those from the int constants, since @link does not resolve nested types the same way javac would, unfortunately.)


[JG20] "@return one of the constants in this class" is not correct for methods returning an enum; can just say something generic like "@return an answer or {@code UNKNOWN}"


[JG21] ProjectSharabilityQuery should use @SuppressWarnings("deprecation") since it is intentionally implementing the old interface. The same might need to be done in the query API classes which call the old interface.


[JG22] @see tags on the query API classes should be made to link to the new SPI interfaces.
Comment 33 Alexander Simon 2012-01-10 08:36:31 UTC
Created attachment 114750 [details]
variant #7 (fixed JG16-JG22)
Comment 34 Alexander Simon 2012-01-10 08:51:38 UTC
Created attachment 114752 [details]
variant #7 (fixed JG16-JG22)
Comment 35 Alexander Simon 2012-01-11 15:21:48 UTC
If there are no objections, let's integrate Friday.
Comment 36 Jesse Glick 2012-01-11 16:46:18 UTC
Still need JG14 addressed but probably I can lead this in a separate API review. Everything else looks fine as far as I can tell.
Comment 37 Alexander Simon 2012-01-12 10:12:51 UTC
(In reply to comment #36)
> Still need JG14 addressed but probably I can lead this in a separate API
> review. Everything else looks fine as far as I can tell.
Thanks, will integrate tomorrow.
Comment 38 Alexander Simon 2012-01-13 09:14:33 UTC
fixed, change set:
http://hg.netbeans.org/cnd-main/rev/759d3ba12c96
Comment 39 Quality Engineering 2012-01-13 21:51:59 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/759d3ba12c96
User: Alexander Simon <alexvsimon@netbeans.org>
Log: fixed Bug #198060 SharabilityQuery / SharabilityQueryImplementation should be extended to use FileObject


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo