Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a relative base URL to allow hosting of app in OMERO.web #59

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

will-moore
Copy link
Contributor

@will-moore will-moore commented Jun 14, 2023

@xinaesthete This is the tweak I needed to your branch to make it work in omero-mdv, since the app is hosted at server/omero-mdv/ and all the static assets need to be served at e.g. server/omero-mdv/mdv.js rather than server/mdv.js.
Hopefully this change shouldn't affect the way you're deploying to netlify etc?

If this change is deployed to e.g. netlify then I could try loading the app on the fly from there, as a possible alternative to serving the assets from omero-web itself.

UPDATE: in this branch, I have subsequently tried to merge in the pjt-dev branch which contains various changes and I haven't yet managed to get this to work with OMERO-mdv.

So, I have checked-out a new branch at https://github.com/will-moore/MDV/tree/omero-mdv-build to keep track of the commit that I'm successfully using in omero-mdv.

Cheers,
Will

@netlify
Copy link

netlify bot commented Jun 14, 2023

Deploy Preview for mdv-dev ready!

Name Link
🔨 Latest commit c7976aa
🔍 Latest deploy log https://app.netlify.com/sites/mdv-dev/deploys/656090fdb2763300084c0245
😎 Deploy Preview https://deploy-preview-59--mdv-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

This simply saves you having to enter the dir when the page is refreshed, e.g. during development
@will-moore
Copy link
Contributor Author

@xinaesthete I realise that this branch (which I'm currently using in OMERO-MDV) was getting quite a long way behind your branch (and had merge conflicts) so I tried to resolve those conflicts and tested the resulting branch build in OMERO-MDV.

It looks like there's a few things that have changed so this isn't currently working with OMERO-MDV.
E.g. I'm getting a 404 for serviceworker.js and the URLs for loading bytes are now .gz so they give a 404 since I'm only handling .b URLs.

So I'm going to hold off on the rebase just now. Maybe we can discuss the best way to handle this process at some point, since it would be great to be able to keep up to date with the latest MDV features etc. Thx.

@xinaesthete
Copy link
Collaborator

Hi Will,

Yes - I've been procrastinating about sorting out some aspects of my build, partly to do with how serviceworker.js is handled (I have a feeling I may have even mentioned that in our call); indeed, I'm somewhat hopeful that when I review your changes they might be helpful to me there... should be trivial, just a bit tedious.

So, I'll review now, do whatever merging of main is needed into my branch, and may make some equivalent changes to yours that will hopefully work better for all of us...

Best,
Peter

xinaesthete added a commit that referenced this pull request Nov 23, 2023
@xinaesthete
Copy link
Collaborator

Pretty sure there are no issues with Netlify & the relative base. I haven't been using or checking that very frequently, so it's always nice to realise it hasn't broken. I'll review your other changes and comments now.

@will-moore
Copy link
Contributor Author

Thanks - I think that only the first commit e8dae46 can be cleanly merged into your branch.

The 2nd commit 676ca1e I don't really need (and it conflicts with your branch). It was just for convenience when running MDV from the vite dev server, to avoid you having to enter a source dir value every time you refreshed the page.
But I'm not using it in OMERO-MDV.

The other commits I am using, to save views back to OMERO, including a CSRF token (I inject that into the page when I serve it) - see https://github.com/will-moore/omero-mdv/blob/709c5841dc2cc161d71d5c82ba7398cc927f78e5/omero_mdv/views.py#L366

I see that you have added a "save state" listener to POST the state, but it looks a bit different from the one I added (I think I was copying from the main branch or somewhere?), so I haven't yet checked if the format of the data is the same for both.

The CSRF functionality should just do nothing if there's no CSRF token injected into the page. Would you be happy to merge that logic into MDV, with it being kinda specific to OMERO-MDV? Or is there a nicer way to achieve the same thing?

Is the MDV app expecting the same content for bytes URLs ..gz as it was previously expecting for ..b URLs previously?
I can update OMERO-MDV to handle *.gz URLs, but I'm not sure if I need to change the response since I'm already using gzip.compress(arr.tobytes()) to generate those.

@will-moore
Copy link
Contributor Author

I'll try again to resolve the merge conflicts and leave just the changes that I need...

@xinaesthete
Copy link
Collaborator

Thanks - I think that only the first commit e8dae46 can be cleanly merged into your branch.

The 2nd commit 676ca1e I don't really need (and it conflicts with your branch). It was just for convenience when running MDV from the vite dev server, to avoid you having to enter a source dir value every time you refreshed the page. But I'm not using it in OMERO-MDV.

Yes... I started making a rebase of your branch locally and resolved that, no big deal I don't think, but also I think it's probably better for you to carry on with yours and then I can merge that.

The other commits I am using, to save views back to OMERO, including a CSRF token (I inject that into the page when I serve it) - see https://github.com/will-moore/omero-mdv/blob/709c5841dc2cc161d71d5c82ba7398cc927f78e5/omero_mdv/views.py#L366

I see that you have added a "save state" listener to POST the state, but it looks a bit different from the one I added (I think I was copying from the main branch or somewhere?), so I haven't yet checked if the format of the data is the same for both.

The CSRF functionality should just do nothing if there's no CSRF token injected into the page. Would you be happy to merge that logic into MDV, with it being kinda specific to OMERO-MDV? Or is there a nicer way to achieve the same thing?

I haven't given this much thought, I think it'd be fine to merge a version with the CSRF token.

Is the MDV app expecting the same content for bytes URLs ..gz as it was previously expecting for ..b URLs previously? I can update OMERO-MDV to handle *.gz URLs, but I'm not sure if I need to change the response since I'm already using gzip.compress(arr.tobytes()) to generate those.

As far as I know there is no difference other than the extension for .gz & .b (I've tried to make it so that compatibility with old projects wasn't broken at least on my branch, not entirely sure that's true of main).

@will-moore
Copy link
Contributor Author

The rebased app seems to be mostly working in OMERO-MDV.
One thing that seems broken is the URLs that MDV is requesting thumbnails and images on.

The datasources.js has sections like:

"images": {
            "composites": {
                "base_url": "./images/",
                "type": "png",
                "key_column": "Image"
            }
        },
        "large_images": {
            "composites": {
                "base_url": "./images/",
                "type": "png",
                "key_column": "Image"
            }
        },

and the page is hosted at http://localhost:4080/omero_mdv/
but the URLs that are being requested have a double // that wasn't there before, e.g:

http://localhost:4080/omero_mdv/config/19673//thumbnail/706.png

Do I need to provide a different datasources.js config or something else?
(I can update the OMERO-MDV to handle URLs with double // but it doesn't feel right).
Thanks

@will-moore
Copy link
Contributor Author

Another issue I'm finding is with "Save" of views back to OMERO.
Currently the MDV app is hosted within OMERO.web at a URL like: http://localhost:4080/omero_mdv/?dir=config/19673/
so the staticFolder variable is True, which disables the Save at

if (type === "state_saved" && !staticFolder) {

Maybe I should try to use a different 'dir' URL so that staicFolder is False.
const staticFolder = !dir.startsWith("/project");
I'll look into it...

@will-moore
Copy link
Contributor Author

So I resolved the merge conflicts which leaves nothing to change!

Now I'm adding more changes to this branch that I need to get the updated MDV app to work with OMERO-MDV again (this will include CSRF changes), and at the same time I'm updating OMERO-MDV at will-moore/omero-mdv#9

The first of these changes is to use ?dir=project/ID instead of ?dir=config/ID, so that staticFolder is False. Unfortunately I still needed to make an edit above, since I can't start the dir with a /.
If I use ?dir=/project/ID then the app at <my-omero-server>/omero_mdv/?dir=project/19673/ will try to load the datasources etc from <my-omero-server>/project/19673/datasources.json which is not within the omero_mdv app so this is 404. Without the slash this is working at <my-omero-server>/omero_mdv/project/19673/datasources.json.

@will-moore
Copy link
Contributor Author

will-moore commented Nov 24, 2023

The next issue I'm currently stuck on is that the MDV app is sending a POST requests to /omero_mdv/project/19673/get_view and /omero_mdv/project/19673/get_data which I've not dealt with before.
I've added CSRF tokens to these requests (see commit above) and
I've made a guess on how to respond to the get_view request, and this seems to be working.
But I'm not sure how to handle the get_data request? What response is expected for that?
The request contains JSON like

{'columns': [{'name': 'Image', 'field': 'Image', 'datatype': 'integer'}, {'name': 'webclient link', 'field': 'webclient link', 'datatype': 'text16'}, {'name': 'Dataset_Name', 'field': 'Dataset_Name', 'datatype': 'text'}], 'data_source': 'mdv_config_19673'}

so it looks like it's expecting binary data for multiple columns, but I'm not sure how to combine them?

@xinaesthete
Copy link
Collaborator

Hi Will,

Hopefully much of that will be fairly straightforward (famous last words), I'll review your comments in more detail soon.

Best wishes

@martinSergeant
Copy link
Contributor

I just had a look at this thread want to add some comments:-

The serviceworker.js is just for static pages - it adds the headers to allow webworkers. Its just for static servers e.g. githubio (I also use it for a system we have in the WIMM , where you can put files in certain directory and they will be publicly served). It is not intended to be used with a 'proper backend' where you have control over which response headers that can be added

I wanted to keep the backend and frontend as decoupled as possible, the dektop.config.js will build code that interacts with either the lightweight local server or static files and is bundled into the lightweight python server and also included when you create a static page. Again this code is not intended for a 'functional' server

The prod.config.js (npm run build) builds a js file that exposes the ChartManager and a few generic methods (in the mdv 'namespace') for use with a dataloader. I imagine that people's use cases will be different so have left it up to the individual to handle loading and saving data.

In the backend that I am developing at the moment, I have the the following code (no csrf tokens yet), which is similar to your code:-

  async function loadMDV(div,pid,name,url,csfr_token){
      
      async function executeProjectAction(method,args,ret_type="json"){
          args["name"]=name
          const formData =  new FormData();
          formData.append("pid",pid);
          formData.append("method",method);
          formData.append("args",JSON.stringify(args));
          const resp = await fetch(url,{method:"POST",body:formData});
          if (ret_type=="bytes"){
              return await resp.arrayBuffer();
          }
          return await resp.json();
      }
  
      const configs = await executeProjectAction("get_mdv_configs",{});
  
      const dataLoader={
          function:async (columns,datasource,size)=>{
              const data = await executeProjectAction("get_mdv_data",{columns,datasource},"bytes");
              return mdv.processArrayBuffer(data,columns,size);
          },
          viewLoader:async(view) =>{
              return await executeProjectAction("get_mdv_view",{view});
          }
          //need to add row data loader and arbitrary binary data loader
      }
  
      const listener = (type,cm,data)=>{
          switch(type){
              case "state_saved":
                  executeProjectAction("save_mdv_state",data);
                  break;  
              }
      }
      const cm = new mdv.ChartManager(div,configs.datasources,dataLoader,configs.state,listener);   
  }

The backend just does user authentication and then calls methods on an MDVProject e,g, to get data
p.get_byte_data(columns,group) , which returns concatenated binary data

@xinaesthete
Copy link
Collaborator

I've been making some more changes to how the vite config works - so now the entry is not an html file, and this may somewhat change how @will-moore uses it... although that said, my latest changes will also break my netlify build. So there is a risk that previously working scripts may break temporarily - LMK if there's any problems. Once the dust settles, it may make sense for OMERO-MDV to slightly change how it incorporates new versions - hopefully will be an improvement (maybe marginal).

We should also figure out the best ways for you to configure data-loaders etc there...

@xinaesthete
Copy link
Collaborator

@will-moore I'm trying to do a bit of housekeeping and of course some things have changed a lot since you made this PR, but I'm making a bit of a note-to-self that you had relevant changes to do with CSRF_TOKENS that might warrant us looking at in the near future. We've been meaning to check in with OME a bit more in general - hopefully catch up some time.

Regards,
Peter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants