Bug 192595 - @EntityCatalogRegistration
@EntityCatalogRegistration
Status: RESOLVED WONTFIX
Product: platform
Classification: Unclassified
Component: -- Other --
7.0
All All
: P3 (vote)
: TBD
Assigned To: Antonin Nebuzelsky
issues@platform
: API
: 35880 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-28 15:38 UTC by Jesse Glick
Modified: 2014-04-11 10:14 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Proposed patch, minus apichanges (39.13 KB, patch)
2010-11-28 16:47 UTC, Jesse Glick
Details | Diff
Slightly updated patch, in case this is still useful (39.43 KB, patch)
2011-01-20 20:32 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2010-11-28 15:38:09 UTC
Currently registering a DTD in the system is cumbersome and error-prone; you have to follow some rather complex instructions in EntityCatalog and create a layer entry. This is a natural candidate for an annotation to hide the details.

Making such an annotation should be simple, though I am not sure where to put it. Ideally the annotation, and its processor, would be collocated with the runtime implementation. EntityCatalog is in openide.util and defines the placement, but cannot actually implement it because this uses layers. FileEntityResolver in openide.loaders currently implements it, but this seems wrong since the impl does not need anything else in Datasystems. openide.filesystems is the "lowest" possible place the impl could go, though sticking this annotation in the org.openide.filesystems package seems odd.
Comment 1 Jesse Glick 2010-11-28 16:47:22 UTC
Created attachment 103416 [details]
Proposed patch, minus apichanges
Comment 2 Jesse Glick 2010-11-28 16:47:53 UTC
Please review.
Comment 3 Jaroslav Tulach 2010-11-29 10:58:57 UTC
Y01 Stuffing this into openide.filesystems seems wrong to me. I'd not be afraid to start new module org.netbeans.api.dtd or even reuse existing org.netbeans.api.xml (and move it to platform).

Such module can easilly have compile time dependency on filesystems and host any XML related annotations while having low closure of runtime dependencies. Existing utilities like org.openide.xml could be slowly migrated in there.

Potentially the api.xml current content (probably requires dependency on openide.nodes) could be moved away and the org.netbeans.api.xml code name used solely for purposes of new, modern APIs for dealing with XML.
Comment 4 Jesse Glick 2010-11-29 15:44:14 UTC
(In reply to comment #3)
> Existing utilities like org.openide.xml could be slowly migrated in there.

I don't see any way of migrating org.openide.xml upwards. Even if e.g. XMLUtil were copied into some new module, the old version could not delegate to the new version, so what would be the point?

(Module auto deps could be used to redirect existing clients, but this would still be an incompatible change at the source level, which is a major disturbance for such a widely used module as openide.util. Anyway XMLUtil is used in a number of places in openide.filesystems.)

> Potentially the api.xml current content (probably requires dependency on
> openide.nodes)

And openide.loaders, and openide.text; and these dependencies are mentioned in public signatures. So there is no good way to minimize its dependencies. Anyway api.xml is used by IDE modules which work with XML document editors, which does not really belong in the platform and is a much more specialized task than registering entities in the system catalog to avoid network connections from XMLUtil.parse.

If openide.filesystems is unacceptable (even under a different package name), then I would rather put the annotation where the runtime currently resides, in openide.loaders. It would be possible to move this "down" into openide.filesystems or some new module TBD later.
Comment 5 Jaroslav Tulach 2010-12-01 18:30:51 UTC
(In reply to comment #4)
> I don't see any way of migrating org.openide.xml upwards. Even if e.g. XMLUtil
> were copied into some new module, the old version could not delegate to the 
> new
> version, so what would be the point?

The point if that people would start to use the new API migrating way from the original (misplaced) one. Creating the new API methods is the visible part of the change. How do we make the old XMLUtil and new API behave the same is implementation issue. 

> but this would
> still be an incompatible change at the source level, 

Option1: XMLUtil finds its own implementation in lookup (provided by the xml module)

Option2: We duplicate the code. Share the tests to ensure the code behaves the same.

Option3: New API delegates to old XMLUtils. In future we switch to option 1 or option 2.

> which is a major
> disturbance for such a widely used module as openide.util. Anyway XMLUtil is
> used in a number of places in openide.filesystems.)

That is indeed annoying. Option 3 would however work OK for this case.

> > Potentially the api.xml current content (probably requires dependency on
> > openide.nodes)
> 
> And openide.loaders, and openide.text; and these dependencies are mentioned in
> public signatures. So there is no good way to minimize its dependencies. Anyway
> api.xml is used by IDE modules which work with XML document editors, which does
> not really belong in the platform and is a much more specialized task than
> registering entities in the system catalog to avoid network connections from
> XMLUtil.parse.

OK. I just liked the codenamebase. Call the new module org.netbeans.api.xmlutil if you want.

> If openide.filesystems is unacceptable (even under a different package name),
> then I would rather put the annotation where the runtime currently resides, in
> openide.filesystems or some new module TBD later.

OK. Different package name (using the new org.netbeans.api style) is imho a TCR. New module like the above mentioned api.xmlutil is a TCA. 


> openide.loaders. It would be possible to move this "down" into

No! To not encourage new dependencies on openide.loaders is a TCR, imho.
Comment 6 Jesse Glick 2010-12-01 18:58:58 UTC
(In reply to comment #5)
> Option3: New API delegates to old XMLUtils. In future we switch to option 1 or
> option 2.

This seems most attractive in general, but the devil is in the details.

For the case of XMLUtil, the problem is that there are a number of usages of XMLUtil in openide.filesystems and core.startup. Either we:

3a. Make openide.filesystems and core.startup depend on the new module, which must then be in core/*.jar. May not even work, since the new module must depend on openide.filesystems to get LayerGeneratingProcessor.

3b. Copy parts of XMLUtil into a private class in openide.filesystems, and copy other parts into a private class in core.startup. (Some aspects, esp. namespace support, can probably be omitted.)

For the case of EntityCatalog, the problem is that this is defined as an SPI, i.e. you could in principle register a subtype into global lookup. Either we:

3c. Make the new EntityCatalog be a subtype of the old one, which prevents eventual abandonment of the old one and looks ugly since it will inherit from a deprecated type.

3d. Search lookup for instances of either kind of EntityCatalog, which will not work for the case of an instance of the new EC when the old EC.getDefault() is called.

Compared to these obstacles, plus the overhead of a new module and the labor involved in getting hundreds of client classes to switch to a new import, the current patch seems much more straightforward.
Comment 7 Jesse Glick 2010-12-01 21:13:19 UTC
(In reply to comment #6)
> 3b. Copy parts of XMLUtil into a private class

To clarify: this would be needed when finally deprecating XMLUtil, not when introducing a delegating copy (can use @SuppressWarnings("deprecation") in the meantime).

> For the case of EntityCatalog, the problem is that this is defined as an SPI

or

3e. Do not support custom instances of the new EC in the global lookup at all; continue to support instances of the old EC until this is deleted. (The support for layer registration can be hardcoded, and the other instance in current global lookup is from openide.loaders and only deals with long-deprecated APIs.)
Comment 8 Jesse Glick 2010-12-06 15:49:27 UTC
Since the original patch has been sidetracked by a request to move even the unrelated XMLUtil into a new module, which is a much more disruptive change than just replacing a layer registration with an annotation, I will try a different approach: @EntityCatalog.Registration in openide.util, with processor and runtime impl there as well, using e.g.

---%<--- .../org-openide-loaders.jar!/META-INF/entities
-//NetBeans//Entity Mapping Registration 1.0//EN
org/netbeans/modules/openide/loaders/EntityCatalog.dtd
-//NetBeans IDE//DTD xmlinfo//EN
org/netbeans/modules/openide/loaders/xmlinfo.dtd
---%<---

and a new public method Set<String> publicIDs() in EntityCatalog. Should be straightforward and permit the annotation to live in the most natural place. The current xml/entities/ registration can be deprecated and the runtime impl can continue to live in openide.loaders.
Comment 9 Jaroslav Tulach 2010-12-07 17:20:04 UTC
Don't give up! I did not want to block the original attempt, just make it better. I understand the problems deprecating XMLUtil, so I am going to give up on that.

Still I believe new module is better than openide.filesystems or openide.loaders. 

I prefer use of LayerGeneratingProcessor over other configuration files.

So just create xml.catalog (I know it exists) or xml.catalog.api module and put your @EntityCatalogRegistration and its processor there. Make this module depend on openide.filesystems and let everyone who wants to register the catalog use this new module.

My primary vision is to move (most of) the XML related stuff into autoload modules and eliminate that from the fixed ones.
Comment 10 Jesse Glick 2010-12-07 18:34:43 UTC
After some offline discussion: xml.catalog.api is reasonable but only if a cleaned-up version of what is now in the SPI of xml.catalog gets moved there, so it has some interesting content beyond what EntityCatalog does now. We probably want three annotations (plus plural variants):

1. @Catalog(id="projects", displayName="#...", shortDescription="#...", icon="...") to create a read-only XML catalog (displayed in Tools > DTDs and XML Schemas). For example, project.ant would do this instead of the current ProjectXMLCatalogReader.

2. @Entity(publicID="...", entity="something.dtd") for registering DTDs for internal use (such as for settings), similar to former proposal. Optional attr catalog="maven" to create an alternate entity catalog useful for user documents.

3. @Schema(uri="...", schema="something.xsd", catalog="projects") for registering schemas by namespace URI to a catalog of your choice. For example, various project type modules would use this to register schemas into project.ant's catalog.

EntityCatalog.getDefault could probably stay where it is (the new module would register the preferred EntityCatalog impl), and XMLUtil need not be touched, but there could be a new API such as:

public abstract class Catalog {
    public static final String ID_INTERNAL; // used for EC.default
    public static @CheckForNull Catalog forID(String id);
    public static Set<Catalog> allCatalogs(); // in Lookup
    public abstract String getID();
    public abstract String getDisplayName();
    public abstract String getShortDescription();
    public abstract Icon getIcon();
    public abstract EntityResolver createEntityResolver(); // cf. XMLUtil.parse
    public abstract Schema createSchema(); // cf. XMLUtil.validate
}

where the impl of createSchema in the standard catalog impl (created using annotations above) would either eagerly combine all schemas as ProjectXMLCatalogReader does today, or use LSResourceResolver to load a schema on demand if that proves to work. (MavenCatalog also uses some trick with "SCHEMA:" prefix that needs to be investigated.)

Some sort of compatibility layer for users of xml.catalog's SPI would be desirable; TBD what we can do.
Comment 11 Jesse Glick 2011-01-20 20:32:09 UTC
Created attachment 105213 [details]
Slightly updated patch, in case this is still useful
Comment 12 Jesse Glick 2011-03-02 17:37:58 UTC
*** Bug 35880 has been marked as a duplicate of this bug. ***
Comment 13 Jesse Glick 2012-03-06 14:19:28 UTC
Too much for 7.2.
Comment 14 Jaroslav Tulach 2014-04-11 10:14:31 UTC
Any interest in pushing this forward?


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