[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #862: Expose the DFDLCatalogResolver to the SAPI/JAPI

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org