stevedlawrence commented on code in PR #862:
URL: https://github.com/apache/daffodil/pull/862#discussion_r1002100915
##########
daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala:
##########
@@ -1089,3 +1090,18 @@ class DaffodilUnparseContentHandler private[sapi]
(sContentHandler: SDaffodilUnp
override def skippedEntity(name: String): Unit =
contentHandler.skippedEntity(name)
}
+
+/**
+ * Exposes access to the DFDLCatalogResolver to the Java API.
Review Comment:
DFDLCatalogResolver is really an internal thing that most users aren't going
to know about, so describing this as a way to get access to that might not be
all that helpful. I think we can just remove this line?
##########
daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala:
##########
@@ -1089,3 +1090,18 @@ class DaffodilUnparseContentHandler private[sapi]
(sContentHandler: SDaffodilUnp
override def skippedEntity(name: String): Unit =
contentHandler.skippedEntity(name)
}
+
+/**
+ * Exposes access to the DFDLCatalogResolver to the Java API.
+ *
+ * Resolves URI/URL/URNs to loadable files/streams.
Review Comment:
Should way make it clear that this thing is what Daffodil uses to resolve
entities? E.g.
> Returns the EntityResolver used by Daffodil to resolve import/include
schemaLocations
The idea being that if you are using DFDL schemas in outside of Daffodil and
you need Daffodil's same behavior, then this is what you should use.
Also, should we be specific how this thing we return actually resolves
entities? For example, it looks at the catalog manager, it looks at class
paths, it looks at relative paths, etc. I'm not sure exactly what the logic is,
but this feels like the right place for that documentation, and would be useful
information for people using this class and know how Daffodil actually resolves
entities. It would also resolve
[DAFFODIL-2616](https://issues.apache.org/jira/browse/DAFFODIL-2616)
##########
daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala:
##########
@@ -1089,3 +1090,18 @@ class DaffodilUnparseContentHandler private[sapi]
(sContentHandler: SDaffodilUnp
override def skippedEntity(name: String): Unit =
contentHandler.skippedEntity(name)
}
+
+/**
+ * Exposes access to the DFDLCatalogResolver to the Java API.
+ *
+ * Resolves URI/URL/URNs to loadable files/streams.
+ *
+ * The DFDLCatalogResolver implements the following interfaces:
+ * - org.apache.xerces.xni.parser.XMLEntityResolver
+ * - org.w3c.dom.ls.LSResourceResolver
+ * - org.apache.sax.EntityResolver
+ * - org.apache.sax.ext.EntityResolver2
Review Comment:
I'm not sure listing the interfaces is particularly helpful, especially
since most people probably don't care too much about them aside from
EntityResolver?
Maybe instead we just change the get function to look something like this:
```scala
def get: EntityResolver = DFDLCatalogResolver.get
```
That way it is self documenting that this returns an `EntityResolver`.
Though, would a user ever need one of the other resolver interfaces? I would
guess no? But if so, maybe we have additional functions that return each one
that a user might need? E.g.
```
def getEntityResolver: EntityResolver = DFDLCatalogResolver.get
def getXMLEntityResolver: XMLEntityResolver = DFDLCatalogResolver.get
...
```
Seems a bit verbose, and I'm not sure if that makes sense or not?
Maybe another option is just have a class, e.g.
```scala
class DaffodilXMLEntityResolver extends DFDLCatalogResolver
```
And then maybe the javadoc just shows the interfaces that are implemented?
I'm not sure if it does or not?
##########
daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala:
##########
@@ -1089,3 +1090,18 @@ class DaffodilUnparseContentHandler private[sapi]
(sContentHandler: SDaffodilUnp
override def skippedEntity(name: String): Unit =
contentHandler.skippedEntity(name)
}
+
+/**
+ * Exposes access to the DFDLCatalogResolver to the Java API.
+ *
+ * Resolves URI/URL/URNs to loadable files/streams.
+ *
+ * The DFDLCatalogResolver implements the following interfaces:
+ * - org.apache.xerces.xni.parser.XMLEntityResolver
+ * - org.w3c.dom.ls.LSResourceResolver
+ * - org.apache.sax.EntityResolver
+ * - org.apache.sax.ext.EntityResolver2
+ */
+object DaffodilXMLEntityResolver {
+ def get = DFDLCatalogResolver.get
+}
Review Comment:
Have you checked that the java/scala docs show up reasonably well? I'm not
sure how well comments on objects get converted to java docs.
##########
daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala:
##########
@@ -1089,3 +1090,18 @@ class DaffodilUnparseContentHandler private[sapi]
(sContentHandler: SDaffodilUnp
override def skippedEntity(name: String): Unit =
contentHandler.skippedEntity(name)
}
+
+/**
+ * Exposes access to the DFDLCatalogResolver to the Java API.
+ *
+ * Resolves URI/URL/URNs to loadable files/streams.
+ *
+ * The DFDLCatalogResolver implements the following interfaces:
+ * - org.apache.xerces.xni.parser.XMLEntityResolver
+ * - org.w3c.dom.ls.LSResourceResolver
+ * - org.apache.sax.EntityResolver
+ * - org.apache.sax.ext.EntityResolver2
+ */
Review Comment:
I think we need a blurb about thread safety. Something about the returned
EntityResolver not being thread safe, but if this get function is called from
within different threads then it is since it returns a unique `EntityResolver`
per thread instance. I'm not sure the best way to word that, but something like
that feels important.
And thinking about threads, I wonder if we should always return a new
instance, and we just say it's not thread safe? If the user want to make
optimizations like what Daffodil does and store instances in a ThreadLocal then
they can do that themselves. I'm not sure we should be doing that for them and
forcing them into a single instance per thread?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
