Merge lp:~zorba-coders/zorba/bug-988417-block-internal-module into lp:zorba

[Till Westmann]

Enables a user to disable an built-in module. This is done by adding a collection of blocked modules URIs to the static context. This collection is checked before a built-in module is added to the context in the translator.

[Chris Hillery (ceejatec)]

1. To maintain ABI compatibility, you must only add new virtual functions to the end of public classes.

2. I don't like that this mechanism is so different to the way you block non-internal modules (using a URIMapper with DENY_ACCESS). While it would be a bit less efficient, it would be more consistent to call static_context::resolve_uri() at translator.cpp line 2840 solely for the purpose of seeing if zerr::ZXQP0029_URI_ACCESS_DENIED is thrown.

review: Needs Fixing

[Matthias Brantner (matthias-brantner)]

I quite like Chris's idea from bullet 2. I don't think it's a big overhead if it's done once per module import.

[Till Westmann (tillw)]

It seemed to me that the whole point of distinguishing between built-in modules and other modules here was that one does not want to invoke the the existing URI resolution mechanisms. Wanting to keep this spirit of the existing code, I proposed this solution. If that's not needed, then that's fine with me.
(The one thing that I'm not extremely fond of is that we would invoke exceptions to find the answer to a simple question, but I can live with that.)

Obviously going through the URIMapper mechanism would have the great advantage of not forcing the user of the API to find out which modules are "internal" and which ones aren't. This would also make programs written agains this API more robust wrt. to changes in the internal/external classification of modules.

[Chris Hillery (ceejatec)]

IMHO, the point of "built-in modules" was to prevent the overhead of *compiling* those modules, not bypassing URI resolution per se. It is true, though, that invoking URI resolution implies a bit of performance overhead. I believe that if only the URI mapping stage is invoked that the overhead will be relatively small. It probably is important to bypass the URL Resolution stage, because that's where things like making network connections to download the module might occur.

I agree that using exceptions to communicate results isn't ideal, but I think the code reuse and API consolidation outweighs that concern in this case. However, it would actually be pretty easy to either modify apply_uri_mappers() or provide a slight variant of that method (with appropriate refactoring) which simply checked for DENY_ACCESS and returned a bool instead of throwing an exception. I could do that if you like.

[lp:~zorba-coders/zorba/bug-988417-block-internal-module]

10801. By Till Westmann on 2012-05-01

implement Chris' proposal to implement blocking using static_context::get_candidate_uris

10802. By Till Westmann on 2012-05-01

add tests

10803. By Till Westmann on 2012-05-01

merge trunk

[Till Westmann (tillw)]

I've changed this as proposed (I think) and added some tests.
Having the exception throw directly seems to have the advantage that I get consistent behavior between internal and external modules - so that seems to be good.

As resolution doesn't seem to happen in the current scenario, the only thing that's still not that nice is that we're updating the vector for something that's in principle a read-only operation. But, as you said, that's probably a minor overhead.

[Chris Hillery (ceejatec)]

Ah, yes, I guess you want the exception to be thrown anyway. Cool, that works.

review: Approve

[lp:~zorba-coders/zorba/bug-988417-block-internal-module]

10804. By Till Westmann on 2012-05-04

fix typo

10805. By Till Westmann on 2012-05-04

update changelog

Unmerged revisions

=== modified file 'src/compiler/translator/translator.cpp'
--- src/compiler/translator/translator.cpp	2012-04-26 13:25:13 +0000
+++ src/compiler/translator/translator.cpp	2012-05-01 04:51:21 +0000
@@ -2840,6 +2840,11 @@
   // importing module that X has been imported.
   if (atlist == NULL && static_context::is_builtin_module(targetNS))
   {
+    // hust a test, this will throw, if the access is denied
+    std::vector<zstring> candidateURIs;
+    theRootSctx->get_candidate_uris(targetNS,
+                                    internal::EntityData::MODULE,
+                                    candidateURIs);
     theRootSctx->add_imported_builtin_module(targetNS);
 #ifdef NDEBUG
     // We cannot skip the math or the sctx introspection modules because they

=== modified file 'src/context/static_context.cpp'
--- src/context/static_context.cpp	2012-04-27 10:20:33 +0000
+++ src/context/static_context.cpp	2012-05-01 04:51:21 +0000
@@ -1526,6 +1526,21 @@
   }
 }
 
+void static_context::get_candidate_uris(
+    zstring const& aUri,
+    internal::EntityData::Kind aEntityKind,
+    std::vector<zstring>& oComponents) const
+{
+  // Create a simple EntityData that just reports the specified Kind
+  internal::EntityData const lData(aEntityKind);
+
+  apply_uri_mappers(aUri, &lData, internal::URIMapper::CANDIDATE, oComponents);
+  if (oComponents.size() == 0)
+  {
+    oComponents.push_back(aUri);
+  }
+}
+
 
 /***************************************************************************//**
 

=== modified file 'src/context/static_context.h'
--- src/context/static_context.h	2012-04-24 18:22:23 +0000
+++ src/context/static_context.h	2012-05-01 04:51:21 +0000
@@ -705,6 +705,15 @@
   (zstring const& aUri, internal::EntityData::Kind aEntityKind,
     std::vector<zstring>& oComponents) const;
 
+  /**
+   * Given a URI, populate a vector with a list of candidate URIs.  If
+   * no candidate URIs are available, the vector will be populated
+   * with (only) the input URI.
+   */
+  void get_candidate_uris
+  (zstring const& aUri, internal::EntityData::Kind aEntityKind,
+    std::vector<zstring>& oComponents) const;
+
   void set_uri_path(const std::vector<zstring>& aURIPath);
 
   void get_uri_path(std::vector<zstring>& oURIPath) const;

=== modified file 'test/api/userdefined_uri_resolution.cpp'
--- test/api/userdefined_uri_resolution.cpp	2012-04-24 12:39:38 +0000
+++ test/api/userdefined_uri_resolution.cpp	2012-05-01 04:51:21 +0000
@@ -79,11 +79,13 @@
   virtual ~DenyAccessURIMapper() {}
 
   virtual void mapURI(const zorba::String aUri,
-    EntityData const* aEntityData,
+    EntityData const*,
     std::vector<zorba::String>& oUris) throw ()
   {
     // Deny access to an URI that would otherwise work
-    if(aUri == "http://www.zorba-xquery.com/tutorials/helloworld.xsd") {
+    if(aUri == "http://www.zorba-xquery.com/tutorials/helloworld.xsd" ||
+       aUri == "http://www.zorba-xquery.com/modules/fetch" ||
+       aUri == "http://expath.org/ns/file") {
       oUris.push_back(URIMapper::DENY_ACCESS);
     }
   }
@@ -259,7 +261,53 @@
   return false;
 }
 
-bool test_deny_access(Zorba* aZorba)
+bool test_deny_internal_module_access(Zorba* aZorba)
+{
+  StaticContext_t lContext = aZorba->createStaticContext();
+
+  DenyAccessURIMapper lMapper;
+  lContext->registerURIMapper(&lMapper);
+
+  try {
+    XQuery_t lQuery = aZorba->compileQuery
+      ("import module namespace fetch = "
+        "'http://www.zorba-xquery.com/modules/fetch'; "
+        "1 + 1", lContext);
+    std::cout << lQuery << std::endl;
+  } catch (ZorbaException& e) {
+    std::cout << "Caught exception: " << e.what() << std::endl;
+    if (e.diagnostic() == zerr::ZXQP0029_URI_ACCESS_DENIED) {
+      std::cout << "...the correct exception!" << std::endl;
+      return true;
+    }
+  }
+  return false;
+}
+
+bool test_deny_external_module_access(Zorba* aZorba)
+{
+  StaticContext_t lContext = aZorba->createStaticContext();
+
+  DenyAccessURIMapper lMapper;
+  lContext->registerURIMapper(&lMapper);
+
+  try {
+    XQuery_t lQuery = aZorba->compileQuery
+      ("import module namespace file = "
+        "'http://expath.org/ns/file'; "
+        "1 + 1", lContext);
+    std::cout << lQuery << std::endl;
+  } catch (ZorbaException& e) {
+    std::cout << "Caught exception: " << e.what() << std::endl;
+    if (e.diagnostic() == zerr::ZXQP0029_URI_ACCESS_DENIED) {
+      std::cout << "...the correct exception!" << std::endl;
+      return true;
+    }
+  }
+  return false;
+}
+
+bool test_deny_schema_access(Zorba* aZorba)
 {
   StaticContext_t lContext = aZorba->createStaticContext();
 
@@ -355,8 +403,20 @@
     std::cout << "  ...failed!" << std::endl;
   }
 
-  std::cout << "test_deny_access" << std::endl;
-  if (!test_deny_access(lZorba)) {
+  std::cout << "test_deny_internal_module_access" << std::endl;
+  if (!test_deny_internal_module_access(lZorba)) {
+    retval = 1;
+    std::cout << " ... failed!" << std::endl;
+  }
+
+  std::cout << "test_deny_external_module_access" << std::endl;
+  if (!test_deny_external_module_access(lZorba)) {
+    retval = 1;
+    std::cout << " ... failed!" << std::endl;
+  }
+
+  std::cout << "test_deny_schema_access" << std::endl;
+  if (!test_deny_schema_access(lZorba)) {
     retval = 1;
     std::cout << " ... failed!" << std::endl;
   }