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

Proposed by Till Westmann
Status: Merged
Approved by: Till Westmann
Approved revision: 10805
Merged at revision: 10821
Proposed branch: lp:~zorba-coders/zorba/bug-988417-block-internal-module
Merge into: lp:zorba
Diff against target: 171 lines (+95/-5)
5 files modified
ChangeLog (+1/-0)
src/compiler/translator/translator.cpp (+5/-0)
src/context/static_context.cpp (+15/-0)
src/context/static_context.h (+9/-0)
test/api/userdefined_uri_resolution.cpp (+65/-5)
To merge this branch: bzr merge lp:~zorba-coders/zorba/bug-988417-block-internal-module
Reviewer Review Type Date Requested Status
Matthias Brantner Approve
Markos Zaharioudakis Approve
Chris Hillery Approve
Review via email: mp+104207@code.launchpad.net

This proposal supersedes a proposal from 2012-04-25.

Commit message

enable blocking of internal modules by running through URI mapping (but not through URL resolution) during translation

Description of the change

Enables a user to disable an built-in module. This is done running through URI mapping (but not through URL resolution) during translation.

To post a comment you must log in.
Revision history for this message
Chris Hillery (ceejatec) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Matthias Brantner (matthias-brantner) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Till Westmann (tillw) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Chris Hillery (ceejatec) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Till Westmann (tillw) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Chris Hillery (ceejatec) wrote : Posted in a previous version of this proposal

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

review: Approve
Revision history for this message
Zorba Build Bot (zorba-buildbot) wrote :
Revision history for this message
Zorba Build Bot (zorba-buildbot) wrote :

Validation queue job bug-988417-block-internal-module-2012-05-02T20-19-00.73Z is finished. The final status was:

All tests succeeded!

Revision history for this message
Zorba Build Bot (zorba-buildbot) wrote :

Voting does not meet specified criteria. Required: Approve > 1, Disapprove < 1, Needs Fixing < 1, Pending < 1. Got: 4 Pending.

Revision history for this message
Chris Hillery (ceejatec) :
review: Approve
Revision history for this message
Markos Zaharioudakis (markos-za) :
review: Approve
Revision history for this message
Matthias Brantner (matthias-brantner) wrote :

s/hust/just

review: Approve
10804. By Till Westmann

fix typo

10805. By Till Westmann

update changelog

Revision history for this message
Zorba Build Bot (zorba-buildbot) wrote :
Revision history for this message
Zorba Build Bot (zorba-buildbot) wrote :

Validation queue job bug-988417-block-internal-module-2012-05-05T02-07-02.544Z is finished. The final status was:

All tests succeeded!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2012-05-03 12:31:51 +0000
3+++ ChangeLog 2012-05-04 18:49:21 +0000
4@@ -40,6 +40,7 @@
5 * Fixed bug #966706 (key uniqueness of index not enforced during incremental refresh)
6 * Fixed bug #906494 (default compile with D_FILE_OFFSET_BITS=64)
7 * Fixed bug #988412 (date:current-dateTime daylight saving)
8+ * Fixed bug #988417 (block internal modules)
9 * Fixed bug #912586, #912593 and #912722 (assertion failures with lax validation)
10 * Fixed bug #921458 (file:read-text-lines() blocking)
11 * Fixed bug #981405 (do not hoist expr containing try-catch variables out of the associated try-catch expression)
12
13=== modified file 'src/compiler/translator/translator.cpp'
14--- src/compiler/translator/translator.cpp 2012-05-03 12:31:51 +0000
15+++ src/compiler/translator/translator.cpp 2012-05-04 18:49:21 +0000
16@@ -2844,6 +2844,11 @@
17 // importing module that X has been imported.
18 if (atlist == NULL && static_context::is_builtin_module(targetNS))
19 {
20+ // just a test, this will throw, if the access is denied
21+ std::vector<zstring> candidateURIs;
22+ theRootSctx->get_candidate_uris(targetNS,
23+ internal::EntityData::MODULE,
24+ candidateURIs);
25 theRootSctx->add_imported_builtin_module(targetNS);
26 #ifdef NDEBUG
27 // We cannot skip the math or the sctx introspection modules because they
28
29=== modified file 'src/context/static_context.cpp'
30--- src/context/static_context.cpp 2012-05-03 12:31:51 +0000
31+++ src/context/static_context.cpp 2012-05-04 18:49:21 +0000
32@@ -1526,6 +1526,21 @@
33 }
34 }
35
36+void static_context::get_candidate_uris(
37+ zstring const& aUri,
38+ internal::EntityData::Kind aEntityKind,
39+ std::vector<zstring>& oComponents) const
40+{
41+ // Create a simple EntityData that just reports the specified Kind
42+ internal::EntityData const lData(aEntityKind);
43+
44+ apply_uri_mappers(aUri, &lData, internal::URIMapper::CANDIDATE, oComponents);
45+ if (oComponents.size() == 0)
46+ {
47+ oComponents.push_back(aUri);
48+ }
49+}
50+
51
52 /***************************************************************************//**
53
54
55=== modified file 'src/context/static_context.h'
56--- src/context/static_context.h 2012-05-03 12:31:51 +0000
57+++ src/context/static_context.h 2012-05-04 18:49:21 +0000
58@@ -705,6 +705,15 @@
59 (zstring const& aUri, internal::EntityData::Kind aEntityKind,
60 std::vector<zstring>& oComponents) const;
61
62+ /**
63+ * Given a URI, populate a vector with a list of candidate URIs. If
64+ * no candidate URIs are available, the vector will be populated
65+ * with (only) the input URI.
66+ */
67+ void get_candidate_uris
68+ (zstring const& aUri, internal::EntityData::Kind aEntityKind,
69+ std::vector<zstring>& oComponents) const;
70+
71 void set_uri_path(const std::vector<zstring>& aURIPath);
72
73 void get_uri_path(std::vector<zstring>& oURIPath) const;
74
75=== modified file 'test/api/userdefined_uri_resolution.cpp'
76--- test/api/userdefined_uri_resolution.cpp 2012-05-03 12:31:51 +0000
77+++ test/api/userdefined_uri_resolution.cpp 2012-05-04 18:49:21 +0000
78@@ -79,11 +79,13 @@
79 virtual ~DenyAccessURIMapper() {}
80
81 virtual void mapURI(const zorba::String aUri,
82- EntityData const* aEntityData,
83+ EntityData const*,
84 std::vector<zorba::String>& oUris) throw ()
85 {
86 // Deny access to an URI that would otherwise work
87- if(aUri == "http://www.zorba-xquery.com/tutorials/helloworld.xsd") {
88+ if(aUri == "http://www.zorba-xquery.com/tutorials/helloworld.xsd" ||
89+ aUri == "http://www.zorba-xquery.com/modules/fetch" ||
90+ aUri == "http://expath.org/ns/file") {
91 oUris.push_back(URIMapper::DENY_ACCESS);
92 }
93 }
94@@ -259,7 +261,53 @@
95 return false;
96 }
97
98-bool test_deny_access(Zorba* aZorba)
99+bool test_deny_internal_module_access(Zorba* aZorba)
100+{
101+ StaticContext_t lContext = aZorba->createStaticContext();
102+
103+ DenyAccessURIMapper lMapper;
104+ lContext->registerURIMapper(&lMapper);
105+
106+ try {
107+ XQuery_t lQuery = aZorba->compileQuery
108+ ("import module namespace fetch = "
109+ "'http://www.zorba-xquery.com/modules/fetch'; "
110+ "1 + 1", lContext);
111+ std::cout << lQuery << std::endl;
112+ } catch (ZorbaException& e) {
113+ std::cout << "Caught exception: " << e.what() << std::endl;
114+ if (e.diagnostic() == zerr::ZXQP0029_URI_ACCESS_DENIED) {
115+ std::cout << "...the correct exception!" << std::endl;
116+ return true;
117+ }
118+ }
119+ return false;
120+}
121+
122+bool test_deny_external_module_access(Zorba* aZorba)
123+{
124+ StaticContext_t lContext = aZorba->createStaticContext();
125+
126+ DenyAccessURIMapper lMapper;
127+ lContext->registerURIMapper(&lMapper);
128+
129+ try {
130+ XQuery_t lQuery = aZorba->compileQuery
131+ ("import module namespace file = "
132+ "'http://expath.org/ns/file'; "
133+ "1 + 1", lContext);
134+ std::cout << lQuery << std::endl;
135+ } catch (ZorbaException& e) {
136+ std::cout << "Caught exception: " << e.what() << std::endl;
137+ if (e.diagnostic() == zerr::ZXQP0029_URI_ACCESS_DENIED) {
138+ std::cout << "...the correct exception!" << std::endl;
139+ return true;
140+ }
141+ }
142+ return false;
143+}
144+
145+bool test_deny_schema_access(Zorba* aZorba)
146 {
147 StaticContext_t lContext = aZorba->createStaticContext();
148
149@@ -355,8 +403,20 @@
150 std::cout << " ...failed!" << std::endl;
151 }
152
153- std::cout << "test_deny_access" << std::endl;
154- if (!test_deny_access(lZorba)) {
155+ std::cout << "test_deny_internal_module_access" << std::endl;
156+ if (!test_deny_internal_module_access(lZorba)) {
157+ retval = 1;
158+ std::cout << " ... failed!" << std::endl;
159+ }
160+
161+ std::cout << "test_deny_external_module_access" << std::endl;
162+ if (!test_deny_external_module_access(lZorba)) {
163+ retval = 1;
164+ std::cout << " ... failed!" << std::endl;
165+ }
166+
167+ std::cout << "test_deny_schema_access" << std::endl;
168+ if (!test_deny_schema_access(lZorba)) {
169 retval = 1;
170 std::cout << " ... failed!" << std::endl;
171 }

Subscribers

People subscribed via source and target branches