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.

Bug 75471

Summary: Review of a new API for adding/removing entries from project classpaths
Product: java Reporter: Tomas Zezula <tzezula>
Component: ProjectAssignee: Tomas Zezula <tzezula>
Status: RESOLVED FIXED    
Severity: blocker CC: marcow, markdey, pjiricka, potingwu, ppisl, rnajman
Priority: P3 Keywords: API, API_REVIEW_FAST
Version: 5.x   
Hardware: All   
OS: All   
Issue Type: DEFECT Exception Reporter:
Bug Depends on: 196455    
Bug Blocks: 60852, 73197, 73198, 74356, 75469, 93470    
Attachments: Diff
Complete diff
Diff file
Diff file
No semantic changes, javadoc fixes, changed variable names in implementation

Description Tomas Zezula 2006-04-25 10:40:33 UTC
Currently there is a ClassPathExtender interface which allows modules to add an
element to project's compile classpath.
There are some new requirements which are not covered by ClassPathExtender and a
new API is required to solve them:
1) Web project needs to remove a clashing library before it adds a new web
services library, see #74356
2) Hibernate module requires an API to add entries even on the different
classpath (ClassPath.EXECUTE) than the ClassPath.COMPILE. Currently it access
dirrectly the project properties files to do it, see #75469

The new API solves these requirements, in addition it fixes an ambiguity in the
ProjectClassPathExtender.addArchiveFile, see issue #60852. The
ProjectClassPathExtender is deprecated.

Example of an usage of the new API by client:

Project p;           //Project to be changed
Library oldLibrary;  //Library to be removed
Library newLibrary;  //Library to be added

ProjectClassPathChanger c = p.getLookup().lookup (ProjectClassPathChanger.class);
if (c != null) {
    c.removeLibrary (oldLibrary, ClassPath.COMPILE);
    c.addLibrary (newLibrary, ClassPath.COMPILE);
}
Comment 1 Tomas Zezula 2006-04-25 13:07:21 UTC
Created attachment 30036 [details]
Diff
Comment 2 Tomas Zezula 2006-04-25 13:09:22 UTC
Fixed example of an usage of the new API by client:

Project p;           //Project to be changed
SourceGroup sg;      //SourceGroup identifying a compilation unit
Library oldLibrary;  //Library to be removed
Library newLibrary;  //Library to be added

ProjectClassPathChanger c = p.getLookup().lookup (ProjectClassPathChanger.class);
if (c != null) {
    c.removeLibrary (oldLibrary, sg, ClassPath.COMPILE);
    c.addLibrary (newLibrary, sg, ClassPath.COMPILE);
}
Comment 3 Jesse Glick 2006-04-25 15:31:58 UTC
Suggest "ProjectClassPathModifier" rather than "...Changer".


add/removeRoot should take URL, not FileObject, in case the entry doesn't yet exist.


Need also patches to manifest.mf, apichanges.xml.


Do you have diffs to j2seproject too?
Comment 4 Tomas Zezula 2006-04-25 15:57:31 UTC
I will change the name.
Agree that the URL is better than the FileObject.
I will attach the j2seproject diff + tests tomorrow.
Comment 5 Andrei Badea 2006-05-03 14:38:06 UTC
AB01: in the web and EJB projects we also write some information about the
libraries to project.xml apart from changing the project properties. Then the
project is saved and build-impl.xml regenerated, which takes some time. This has
performance issues when more items need to be added to the classpath, since
project.xml would be saved for each one. Can the methods take a List of items
instead of just one item?

I must admit this is not really necessary now, since the only use case when we
need to add multiple items at once is from within the project type, where we can
(and do) cast to the project-specific impl of ProjectClassPathExtender, which
contains methods accepting arrays. But if we allow the clients to mess with the
project classpath the need to add/remove multiple items from outside the project
may arise, so we could just support it now. 

Some minor stuff:

AB02: ClassPathProvider.getClassPath() refers to ClassPath.COMPILE and friends
as "type", not "id". For the sake of consistency let's use "type" here too.

AB03: The Javadoc could give an example of a case when a method returns false
and doesn't throw an exception. I suppose it can happen when e.g. adding an item
which already exists, but it isn't self-evident.
Comment 6 Tomas Zezula 2006-05-03 15:37:30 UTC
Created attachment 30212 [details]
Complete diff
Comment 7 Tomas Zezula 2006-05-03 15:39:16 UTC
I've attached the complete diff including tests and apichanges.xml. The new diff
also fixes problems reported by Jesse.
Comment 8 Jesse Glick 2006-05-03 17:03:38 UTC
Missing SPL on J2SEProjectClassPathModifierTest.java.
Comment 9 Jaroslav Tulach 2006-05-03 17:23:31 UTC
Y01 I can see that the change deprecates one interface and instead creates new 
one. I'd like to know what are the implications for client code and also what 
are the implications for the implementators? I would expect: project 
providers, stop implementing the old interface and implement the new one. 
Clients - continue to call both of these interfaces if you want to work with 
all projects! If this is correct answer, then I do not like it and I'd suggest 
to split client and provider API.
Comment 10 Tomas Zezula 2006-05-05 12:04:42 UTC
To Andrei: I've incorporated the requirement for handling multiple entities as
well as fixing the javadoc and names of parameters. I will attach the new diff
as soon as the issue reported by Jarda will be resolved.
Comment 11 Tomas Zezula 2006-05-09 17:04:56 UTC
Created attachment 30311 [details]
Diff file
Comment 12 Tomas Zezula 2006-05-09 17:05:32 UTC
Added the diff which solves the API/SPI separation.
Comment 13 Tomas Zezula 2006-05-11 10:39:21 UTC
Another two requirements covered by this API:
http://www.netbeans.org/issues/show_bug.cgi?id=73197
http://www.netbeans.org/issues/show_bug.cgi?id=73198

Comment 14 Jaroslav Tulach 2006-05-11 23:54:44 UTC
Imho, excelent from point of view of next future evolutions. Thanks.

Now a bit of stylistic comments, but as this is a matter of personal style, 
take that as comments, no requests:

I do not like the name Extendable much and also I do not like that it does not 
have any methods to modify the content - e.g. addLibrary, etc.

P3 In fact I would merge the two client API classes ProjectClassPathModifier 
and Extendable into one. findExtendables would be the only static method, the 
rest would be instance methods.

P5 I would change the return type from array to List<? extends Extendable>. 

(P10 as I am not good at inventing names) The name of the class. Why not call 
the merged class ExtendableProjectClassPath?

There were just stylistic comments, do not take them much seriously. However 
what is missing is:

apichange for new class in client API, there is just a change describing the 
addition of the class in service provider API, so please add new one or extend 
the one existing.
Comment 15 Jesse Glick 2006-05-12 01:05:10 UTC
s/extendable/extensible/g


I don't like the Extendable (Extensible) interface in the API. Confusing (client
has to iterate over all instances and do some nontrivial checks) and probably
unnecessary. I would suggest removing it from the API and just exposing the
methods that correspond to what the client very likely wants to do: adjust a
particular source root in a particular CP mode. E.g.

public static boolean addLibraries(Project project, Library[] libraries,
SourceGroup group, String classPathType) throws IOException,
UnsupportedOperationException;

which would

- return true on successful change
- return false if nothing was changed
- throw IOException if storing change failed
- throw UnsupportedOperationException if project does not support this kind of
change (adding libs, modifying this source group, or modifying this CP type)

An alternate signature, even simpler for the client, and in line with what e.g.
the form module would want to do:

public static boolean addLibraries(FileObject referenceFile, Library[]
libraries, String classPathType) throws IOException, UnsupportedOperationException;

where referenceFile can be a Java source, package, or source root, and the
project and source group are looked up from that (FOQ & Sources).
Comment 16 Tomas Zezula 2006-05-14 08:49:40 UTC
The API without extendables is not able to keep at least some backward
compatibility with ProjectClassPathExtender, which was required by Jarda.
The client of ProjectClassPathExtender does not know which SourceGroup(s) the
project type extends, it only knows that it extends ClassPath.COMPILE. This is
the reason why the Extendable (the name may change) was introduced. For the
project type providing the ProjectClassPathModifierImplementation it creates
instance for each extendable classpath and its getSourceGroup returns the source
group. In case when the project type provides only ProjectClassPathExtender the
single Extendable which does not provide SourceGroup is created. Client may use
such an extendable to add classpath elements to ClassPath.Compile. If we want to
keep at least some backward compatibility the object which identifies extendable
classpaths of project is needed.
Comment 17 Jesse Glick 2006-05-14 17:00:31 UTC
If you introduce a new API for clients to call then you are already expecting
client code to be modified to call it. (Otherwise the client cannot work with a
modified project type which only provides the new SPI.) So that client code
might as well pass a SourceGroup (or FileObject), which is in fact probably
_easier_ than the old code was (finding the Project with FOQ and checking its
lookup), since the client probably already has a FileObject it wants to work
with. The static API method would deal with looking for both the new and old SPI
and acting accordingly. I don't see the point of asking clients to change their
calling pattern - to something both more complicated, and semantically closer to
the now-deprecated SPI.
Comment 18 Jaroslav Tulach 2006-05-14 19:04:54 UTC
I guess the problem depends on whether the usecase shown in one javadoc in the 
diff is the only one we want to support right now or if there is another 
usecase. If we expect clients to just:

ProjectClassPathModifier.Extendable extendable = null;
for (ProjectClassPathModifier.Extendable e : extendables) {
  if (ClassPath.COMPILE.equals(e.getClassPathType()) && (e.getSourceGroup() == 
null || sources.equals(e.getSourceGroup()))) {
  extendable = e;
  break;
  }
}
if (extendable != null) {
  ProjectClassPathModifier.addLibrary (library, extendable);
}

Then I guess it make sense to provide some simpler client API method that will 
do the same as Jesse suggested. If this is the case, then imho the diff needs 
just two minor corrections: 1. make Extendable and methods that use it package 
private, 2. introduce the new method that will do exactly what is shown in the 
javadoc.

If, in future a new requirement appears requesting more functionality, we can 
always make the (renamed) Extendable part of the API. That is going to be 
compatible change.
Comment 19 Tomas Zezula 2006-05-15 14:47:40 UTC
Created attachment 30400 [details]
Diff file
Comment 20 Tomas Zezula 2006-05-15 14:52:37 UTC
I've changed the API to make it simpler while keeping backward compatibility.
The Extensible changed to package private, ProjectClassPathModifier method
signatures changed to take FileObject and classpath type.
Comment 21 Andrei Badea 2006-05-15 15:09:22 UTC
I was just writing this when Tomas posted the last comment.

I don't like the Extendable interface either, or rather the amount of code
necessary to add/remove a classpath item. If it is decided to leave Extendable
(or however it will be called) in the API there should at least be some methods
allowing the client to easily find an Extendable based on e.g. the SG and CP type.

Jesse's suggestion of a method getting a Java source/package/root FileObject
parameter makes sense to me and it elegantly covers the "extend the classpath of
this file" use case. As an example, in the J2EE area we have the use case of
"extending" a web module (WebModule in web/webapi) by a web framework
(WebFrameworkProvider in web/webapi), which requires adding the library
providing the web framework to the classpath of the web module's sources.
WebModule has a getJavaSources() method returning an array of source roots,
which we will pass one by one as the fileReference parameter of PCMP.addLibraries().

Another use case is in the web services support, where it seems that the JAX-WS
2.0 library is added to (and probably the JAX-RPC one removed from) the
classpaths of all source groups. But that can be easily achieved by iterating
over all source groups.

Tomasi, you still have "classPathId" instead of "classPathType" in many places
in the last diff, both in the J2SE project implementation and in the API's
Javadoc ("from the classpath of the given id"). Please fix them, otherwise "id"
will get copied to our project types and we will never get rid of it.

"a file which classpath should be extended", "SourceGroup which classpath should
be changed" -> "whose classpath".
Comment 22 Tomas Zezula 2006-05-15 17:02:14 UTC
To Andrei: The new version does not expose Extensible interface, all the public
API methods work with FileObject + classpath type as Jesse proposed. I've fixed
the javadoc and changed the variable names to type even in the implementation.

Comment 23 Tomas Zezula 2006-05-15 17:03:31 UTC
Created attachment 30403 [details]
No semantic changes, javadoc fixes, changed variable names in implementation
Comment 24 Jaroslav Tulach 2006-05-15 20:09:15 UTC
As far as I can tell the solution in the last diff seems to satisfy all 
criteria for evolution and client and provider API separation. I like it. If 
it also functionally works, then I agree with the integration.
Comment 25 Andrei Badea 2006-05-22 10:12:30 UTC
Thank you for fixing the variable names. The solution in the last diff supports
the known use cases and it easy to use for the client. I think it can be integrated.
Comment 26 Tomas Zezula 2006-05-22 16:11:40 UTC
Checking in j2seproject/src/org/netbeans/modules/java/j2seproject/J2SEProject.java;
/cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/J2SEProject.java,v
 <--  J2SEProject.java
new revision: 1.61; previous revision: 1.60
done
Checking in
j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/ClassPathProviderImpl.java;
/cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/ClassPathProviderImpl.java,v
 <--  ClassPathProviderImpl.java
new revision: 1.18; previous revision: 1.17
done
Checking in
j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathExtender.java;
/cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathExtender.java,v
 <--  J2SEProjectClassPathExtender.java
new revision: 1.13; previous revision: 1.12
done
RCS file:
/cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifier.java,v
done
Checking in
j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifier.java;
/cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifier.java,v
 <--  J2SEProjectClassPathModifier.java
initial revision: 1.1
done
RCS file:
/cvs/java/j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifierTest.java,v
done
Checking in
j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifierTest.java;
/cvs/java/j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifierTest.java,v
 <--  J2SEProjectClassPathModifierTest.java
initial revision: 1.1
done
Checking in project/apichanges.xml;
/cvs/java/project/apichanges.xml,v  <--  apichanges.xml
new revision: 1.14; previous revision: 1.13
done
Checking in project/manifest.mf;
/cvs/java/project/manifest.mf,v  <--  manifest.mf
new revision: 1.19; previous revision: 1.18
done
Checking in project/nbproject/project.properties;
/cvs/java/project/nbproject/project.properties,v  <--  project.properties
new revision: 1.21; previous revision: 1.20
done
RCS file:
/cvs/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java,v
done
Checking in
project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java;
/cvs/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java,v
 <--  ProjectClassPathModifier.java
initial revision: 1.1
done
RCS file:
/cvs/java/project/src/org/netbeans/modules/java/project/classpath/ProjectClassPathModifierAccessor.java,v
done
Checking in
project/src/org/netbeans/modules/java/project/classpath/ProjectClassPathModifierAccessor.java;
/cvs/java/project/src/org/netbeans/modules/java/project/classpath/ProjectClassPathModifierAccessor.java,v
 <--  ProjectClassPathModifierAccessor.java
initial revision: 1.1
done
Checking in
project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathExtender.java;
/cvs/java/project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathExtender.java,v
 <--  ProjectClassPathExtender.java
new revision: 1.4; previous revision: 1.3
done
RCS file:
/cvs/java/project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathModifierImplementation.java,v
done
Checking in
project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathModifierImplementation.java;
/cvs/java/project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathModifierImplementation.java,v
 <--  ProjectClassPathModifierImplementation.java
initial revision: 1.1
done
Comment 27 Jesse Glick 2006-05-22 19:03:50 UTC
You forgot to make the new package public! It can't be used.


Also typo: "projectArtfact"


Also not all of the @SuppressWarnings("deprecation") are working for me:

/space/src/nb_all/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java:29:
warning: [deprecation]
org.netbeans.spi.java.project.classpath.ProjectClassPathExtender in
org.netbeans.spi.java.project.classpath has been deprecated
import org.netbeans.spi.java.project.classpath.ProjectClassPathExtender;
/space/src/nb_all/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java:289:
warning: [deprecation]
org.netbeans.spi.java.project.classpath.ProjectClassPathExtender in
org.netbeans.spi.java.project.classpath has been deprecated
        private final ProjectClassPathExtender pcpe;
/space/src/nb_all/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java:303:
warning: [deprecation]
org.netbeans.spi.java.project.classpath.ProjectClassPathExtender in
org.netbeans.spi.java.project.classpath has been deprecated
        private Extensible (final ProjectClassPathExtender pcpe) {
Comment 28 Tomas Zezula 2006-05-24 09:45:59 UTC
Checking in java/project/nbproject/project.xml;
/cvs/java/project/nbproject/project.xml,v  <--  project.xml
new revision: 1.24; previous revision: 1.23
done
Checking in
java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java;
/cvs/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java,v
 <--  ProjectClassPathModifier.java
new revision: 1.2; previous revision: 1.1
done
Checking in ide/golden/public-packages.txt;
/cvs/ide/golden/public-packages.txt,v  <--  public-packages.txt
new revision: 1.54; previous revision: 1.53
done
Comment 29 Tomas Zezula 2006-05-24 09:47:44 UTC
There are still some deprecation warnings (2), which cannot be fixed even by
annotating the whole class by @SuppressWarings ("deprecation").