Merge pull request #8001 from jasongrout/urlresolverpath

Use path string instead of session context in url resolver

packages/docregistry/src/context.ts

@@ -79,9 +79,12 @@ export class Context<T extends DocumentRegistry.IModel>
     this.sessionContext.propertyChanged.connect(this._onSessionChanged, this);
     manager.contents.fileChanged.connect(this._onFileChanged, this);
 
-    this.urlResolver = new RenderMimeRegistry.UrlResolver({
-      session: this.sessionContext,
+    const urlResolver = (this.urlResolver = new RenderMimeRegistry.UrlResolver({
+      path: this._path,
       contents: manager.contents
+    }));
+    this.pathChanged.connect((sender, newPath) => {
+      urlResolver.path = newPath;
     });
   }
 

packages/rendermime/src/registry.ts

@@ -305,26 +305,44 @@ export namespace RenderMimeRegistry {
   }
 
   /**
-   * A default resolver that uses a session and a contents manager.
+   * A default resolver that uses a given reference path and a contents manager.
    */
   export class UrlResolver implements IRenderMime.IResolver {
     /**
-     * Create a new url resolver for a console.
+     * Create a new url resolver.
      */
     constructor(options: IUrlResolverOptions) {
-      this._session = options.session;
+      if (options.path) {
+        this._path = options.path;
+      } else if (options.session) {
+        this._session = options.session;
+      } else {
+        throw new Error(
+          "Either 'path' or 'session' must be given as a constructor option"
+        );
+      }
       this._contents = options.contents;
     }
 
+    /**
+     * The path of the object, from which local urls can be derived.
+     */
+    get path(): string {
+      return this._path ?? this._session.path;
+    }
+    set path(value: string) {
+      this._path = value;
+    }
+
     /**
      * Resolve a relative url to an absolute url path.
      */
-    resolveUrl(url: string): Promise<string> {
+    async resolveUrl(url: string): Promise<string> {
       if (this.isLocal(url)) {
-        const cwd = encodeURI(PathExt.dirname(this._session.path));
+        const cwd = encodeURI(PathExt.dirname(this.path));
         url = PathExt.resolve(cwd, url);
       }
-      return Promise.resolve(url);
+      return url;
     }
 
     /**
@@ -333,12 +351,12 @@ export namespace RenderMimeRegistry {
      * #### Notes
      * This URL may include a query parameter.
      */
-    getDownloadUrl(url: string): Promise<string> {
+    async getDownloadUrl(url: string): Promise<string> {
       if (this.isLocal(url)) {
         // decode url->path before passing to contents api
         return this._contents.getDownloadUrl(decodeURI(url));
       }
-      return Promise.resolve(url);
+      return url;
     }
 
     /**
@@ -356,6 +374,7 @@ export namespace RenderMimeRegistry {
       return URLExt.isLocal(url) || !!this._contents.driveName(path);
     }
 
+    private _path: string;
     private _session: ISessionContext | Session.ISessionConnection;
     private _contents: Contents.IManager;
   }
@@ -364,13 +383,26 @@ export namespace RenderMimeRegistry {
    * The options used to create a UrlResolver.
    */
   export interface IUrlResolverOptions {
+    /**
+     * The path providing context for local urls.
+     *
+     * #### Notes
+     * Either session or path must be given, and path takes precedence.
+     */
+    path?: string;
+
     /**
      * The session used by the resolver.
      *
+     * @deprecated use the `path` option instead and update it as needed.
+     *
      * #### Notes
-     * For convenience, this can be a session context as well.
+     * For convenience, this can be a session context as well. Either session
+     * or path must be given, and path takes precedence.
+     *
+     * TODO: remove this option and make `path` required.
      */
-    session: ISessionContext | Session.ISessionConnection;
+    session?: ISessionContext | Session.ISessionConnection;
 
     /**
      * The contents manager used by the resolver.

tests/test-rendermime/src/registry.spec.ts

@@ -313,16 +313,17 @@ describe('rendermime/registry', () => {
 
     describe('.UrlResolver', () => {
       let manager: ServiceManager;
-      let resolver: RenderMimeRegistry.UrlResolver;
+      let resolverSession: RenderMimeRegistry.UrlResolver;
+      let resolverPath: RenderMimeRegistry.UrlResolver;
       let contents: Contents.IManager;
       let session: Session.ISessionConnection;
       const pathParent = 'has%20üni';
       const urlParent = encodeURI(pathParent);
+      const path = pathParent + '/pr%25 ' + UUID.uuid4();
 
       before(async () => {
         manager = new ServiceManager({ standby: 'never' });
         const drive = new Drive({ name: 'extra' });
-        const path = pathParent + '/pr%25 ' + UUID.uuid4();
         contents = manager.contents;
         contents.addDrive(drive);
         await manager.ready;
@@ -331,10 +332,14 @@ describe('rendermime/registry', () => {
           path: path,
           type: 'test'
         });
-        resolver = new RenderMimeRegistry.UrlResolver({
+        resolverSession = new RenderMimeRegistry.UrlResolver({
           session,
           contents: manager.contents
         });
+        resolverPath = new RenderMimeRegistry.UrlResolver({
+          path: path,
+          contents: manager.contents
+        });
       });
 
       after(() => {
@@ -343,14 +348,65 @@ describe('rendermime/registry', () => {
 
       context('#constructor', () => {
         it('should create a UrlResolver instance', () => {
-          expect(resolver).to.be.an.instanceof(RenderMimeRegistry.UrlResolver);
+          expect(resolverSession).to.be.an.instanceof(
+            RenderMimeRegistry.UrlResolver
+          );
+          expect(resolverPath).to.be.an.instanceof(
+            RenderMimeRegistry.UrlResolver
+          );
+        });
+      });
+
+      context('.path', () => {
+        it('should give precedence to the explicit path over the session', async () => {
+          const resolver = new RenderMimeRegistry.UrlResolver({
+            session: new SessionContext({
+              sessionManager: manager.sessions,
+              specsManager: manager.kernelspecs,
+              path: pathParent + '/pr%25 ' + UUID.uuid4(),
+              kernelPreference: { canStart: false, shouldStart: false }
+            }),
+            contents: manager.contents,
+            path: '/some/path/file.txt'
+          });
+          expect(await resolver.resolveUrl('./foo')).to.equal('some/path/foo');
+        });
+
+        it('should fall back to the session path if only the session is given', () => {
+          expect(resolverSession.path).to.equal(path);
+        });
+
+        it('should allow the path to be changed', async () => {
+          const resolver = new RenderMimeRegistry.UrlResolver({
+            session: new SessionContext({
+              sessionManager: manager.sessions,
+              specsManager: manager.kernelspecs,
+              path: pathParent + '/pr%25 ' + UUID.uuid4(),
+              kernelPreference: { canStart: false, shouldStart: false }
+            }),
+            contents: manager.contents
+          });
+          resolver.path = '/some/path/file.txt';
+          expect(await resolver.resolveUrl('./foo')).to.equal('some/path/foo');
+          const resolver2 = new RenderMimeRegistry.UrlResolver({
+            path: '/some/path/file.txt',
+            contents: manager.contents
+          });
+          resolver2.path = '/other/path/file.txt';
+          expect(await resolver2.resolveUrl('./foo')).to.equal(
+            'other/path/foo'
+          );
         });
       });
 
       context('#resolveUrl()', () => {
         it('should resolve a relative url', async () => {
-          const path = await resolver.resolveUrl('./foo');
-          expect(path).to.equal(urlParent + '/foo');
+          expect(await resolverSession.resolveUrl('./foo')).to.equal(
+            urlParent + '/foo'
+          );
+          expect(await resolverPath.resolveUrl('./foo')).to.equal(
+            urlParent + '/foo'
+          );
         });
 
         it('should resolve a relative url with no active session', async () => {
@@ -368,48 +424,58 @@ describe('rendermime/registry', () => {
         });
 
         it('should ignore urls that have a protocol', async () => {
-          const path = await resolver.resolveUrl('http://foo');
-          expect(path).to.equal('http://foo');
+          expect(await resolverSession.resolveUrl('http://foo')).to.equal(
+            'http://foo'
+          );
+          expect(await resolverPath.resolveUrl('http://foo')).to.equal(
+            'http://foo'
+          );
         });
 
         it('should resolve URLs with escapes', async () => {
-          const url = await resolver.resolveUrl('has%20space');
-          expect(url).to.equal(urlParent + '/has%20space');
+          expect(await resolverSession.resolveUrl('has%20space')).to.equal(
+            urlParent + '/has%20space'
+          );
+          expect(await resolverPath.resolveUrl('has%20space')).to.equal(
+            urlParent + '/has%20space'
+          );
         });
       });
 
       context('#getDownloadUrl()', () => {
         it('should resolve an absolute server url to a download url', async () => {
-          const contextPromise = resolver.getDownloadUrl('foo');
+          const contextPromise = resolverPath.getDownloadUrl('foo');
           const contentsPromise = contents.getDownloadUrl('foo');
           const values = await Promise.all([contextPromise, contentsPromise]);
           expect(values[0]).to.equal(values[1]);
         });
 
         it('should resolve escapes correctly', async () => {
-          const contextPromise = resolver.getDownloadUrl('foo%2520test');
+          const contextPromise = resolverPath.getDownloadUrl('foo%2520test');
           const contentsPromise = contents.getDownloadUrl('foo%20test');
           const values = await Promise.all([contextPromise, contentsPromise]);
           expect(values[0]).to.equal(values[1]);
         });
 
         it('should ignore urls that have a protocol', async () => {
-          const path = await resolver.getDownloadUrl('http://foo');
+          const path = await resolverPath.getDownloadUrl('http://foo');
           expect(path).to.equal('http://foo');
         });
       });
 
       context('#isLocal', () => {
         it('should return true for a registered IDrive`', () => {
-          expect(resolver.isLocal('extra:path/to/file')).to.equal(true);
+          expect(resolverPath.isLocal('extra:path/to/file')).to.equal(true);
         });
 
         it('should return false for an unrecognized Drive`', () => {
-          expect(resolver.isLocal('unregistered:path/to/file')).to.equal(false);
+          expect(resolverPath.isLocal('unregistered:path/to/file')).to.equal(
+            false
+          );
         });
 
         it('should return true for a normal filesystem-like path`', () => {
-          expect(resolver.isLocal('path/to/file')).to.equal(true);
+          expect(resolverPath.isLocal('path/to/file')).to.equal(true);
         });
       });
     });