Klaus/change rest methods to post

klues commented 5 years ago

Because of problems with HTTP PUT requests in Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=1376310 ) we decided to change all PUT requests of our REST API to HTTP POST requests, which are "simple requests" and therefore fix the problem of Firefox (see https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Simple_requests ).

DONE:

All PUT Requests from RestServer.java are now POST
PUT requests are still valid, but deprecated
the path rest/restfunctions returns actions only the POST actions, not the PUT actions anymore
the path rest/restfunctions/withdeprecated returns all actions, including the deprecated PUT functions
areCommunicator.js --> changed everything to POST
ARECommunicator.java --> changed everything to POST

TODO/OPEN:

what to do with areCommunicator.js in WebACS? also change it now? But it could lead to incompatibilities with older ARE versions.
I did not manage to run eu.asterics.rest.javaClient.tester.JavaClient.java -> how to run it?

klues commented 5 years ago

I did not manage to run eu.asterics.rest.javaClient.tester.JavaClient.java -> how to run it? 1) go to folder ARE_RestAPIlibraries 2) ant run jar 3) java -jar asterics.are-REST-client.jar

sabicalija commented 5 years ago

I've changed all PUT requests to POST on default. New release can be installed with:

npm i@asterics/are-rest@0.0.1-alpha.1

I haven't tested any functionality so far: https://github.com/asterics/are-rest/pull/1/commits/e03657a227c16484d37274912b1aeb7a0d219f35. If you encounter any problems, please let me know.

sabicalija commented 5 years ago

@asterics/web-app-utils is now updated too (https://github.com/asterics/web-app-utils/pull/1/commits/89210fbc46499b99a7bd232b4d5049c6b29781f4), using @asterics/are-rest@0.0.1-alpha.1. Install it with

npm i@asterics/web-app-utils@0.0.1-alpha.4

klues commented 5 years ago

tests in eu.asterics.rest.javaClient.tester.JavaClient.java run now without problems
added new REST call /version to retrieve current ARE version

deinhofer commented 5 years ago

@klues I have tested all the functions, works like a charme :+1:

@alija Thanks for the fast implementation. not yet tested

klues commented 5 years ago

whats still missing: updated documentation of REST-API. I assume thats somewhere in the AsTeRICS Docs now?!

deinhofer commented 5 years ago

whats still missing: updated documentation of REST-API. I assume thats somewhere in the AsTeRICS Docs now?!

yes, no need for updating the old (REST_API) documents. The generic doc will be moved to asterics-docs and the API description is now self-documenting with the restfunctions call.

The only thing we could update are the client examples: https://github.com/asterics/AsTeRICS/tree/master/ARE_RestAPIlibraries/clientExample but we would only have to add the /versions call.

sabicalija commented 5 years ago

I've tested @asterics/are-rest@0.0.1-alpha.1 with the current version of the WebACS and chrome. Unfortunately, it didn't work instantly. I had to tweak the webpack configuration a little.

It used to be:

const frontend = {
  target: "web",
  entry: "./src/index.js",
  output: {
    path: resolve(__dirname, "dist"),
    filename: "index.js",
    library: "@asterics/are-rest",
    libraryTarget: "umd"
  },
  module: {
    rules: [frontendRules]
  },
  stats: {
    colors: true
  },
  devtool: "source-map",
  mode: process.env.NODE_ENV,
  optimization: {
    minimizer: [new TerserPlugin({ parallel: true, sourceMap: true })]
  }
};

output.library specifies the name which is used to assign the exported module to, when importing it in a browser environment.

Thus, every line of WebACS which is using the previous areCommunicator.js would have to be prefixed with this["@asterics/are-rest"]. Like:

/* now */
setBaseURI(ACS.areBaseURI + '/rest/');
/* future */
this['@asterics/are-rest'].setBaseURI(ACS.areBaseURI + '/rest/');

Assigning functions to the global namespace isn't good practice anyway - is it? What are your thoughts? But this ugly prefix (i.e. this['@asterics/are-rest']) isn't any relief either. :confused:

There is a way to configure webpack to expose the functions provided by @asterics/are-rest to the global namespace:

const frontend = {
  target: "web",
  entry: "./src/index.js",
  output: {
    path: resolve(__dirname, "dist"),
    filename: "index.js",
    library: this,
    libraryTarget: "umd"
  },
  module: {
    rules: [frontendRules]
  },
  stats: {
    colors: true
  },
  devtool: "source-map",
  mode: process.env.NODE_ENV,
  optimization: {
    minimizer: [new TerserPlugin({ parallel: true, sourceMap: true })]
  }
};

which is IMO rather a "hack". :blush: I've tested it already. The only changes that were necessary was changing output.library to this and changing the imported script. Instead of using the file of the repository as it used to be, simply use the unpkg links for npm modules:

<!-- <script type="text/javascript" language="javascript" src="lib/rest_library/areCommunicator.js"></script> -->
<script type="text/javascript" language="javascript" src="https://unpkg.com/@asterics/are-rest@0.0.1-alpha.1/dist/index.js"></script>

I haven't published a version of the last webpack configuration. So you cannot test it at your devices unless we publish an update. However, unfortunately, it does not work anyway - currently. :confused: But the error message is actually quite promising.. :grin:

As you can see in the following pictures, the functions are now exposed to the global browser namespace:

However, the request fails due to a CORS policy violation and blocked by a preflight response:

which is actually the same error message we got earlier with firefox. Am I right? I've tested the WebACS again with the new https server.

Then I realized that I was accessing the http-server over my ethernet connection (i.e. 10.0.0.2:8080). But the 2nd test using the localhost failed too (http -> https):

A final test, with the http REST server again, and localhost failed as well:

I'm a little bit surprised. Does anyone know, why this preflight request is failing but apparently working with ajax request?

EDIT: Is it possible that the preflight request fails with POST but not with PUT requests, due to semantical differences of these methods?

What are your thoughts on the webpack configuration? Should we expose it globally?? Or rather with a user-friendly name, like areREST or areRest? This way, one could use it like:

areREST.setBaseURI(...)

I like this approach probably the most. However, we have to change the lines where areCommunicator.js is used. This, however, is only relevant for the unpkg link-way, but not relevant for changes coming with alija/feature/webpack because there we can easily import the functions and bundle WebACS with webpack, where we do not have this global namespace issue.

klues commented 5 years ago

regarding the question of exposing all methods to global namespace: To my mind this is not a good idea. If every library would do this, the global namespace would become polluted rapidly and collisions will occur very soon.

I see two possibilities: 1) I think the modern approach would be to write the library as a module which exports an object that has all the needed methods that are now in areCommunicator.js. So anyone could include the script with <script type="module" src="yourlib.js"></script> and use it with import {myCustomName} from "yourlib.js" in an other script. However not all browsers are supporting modules, so 2) the existing approach with @asterics/are-rest as a global object is also fine, all big libraries like e.g. JQuery are doing it this way (global $ object). However the name is a littlebit odd with the @ and / character in it, isn't it? Or is it some kind of standard naming convention? I could imagine creating a asterics object in the global namespace and each library creates a sub-object like asterics.rest including the methods of the current @asterics/are-rest library.

sabicalija commented 5 years ago

I like your last suggestion. The name is quite odd, yes that's true. It's just the name of the npm module itself, because I simply did not understand the effects of the webpack configuration option.library before. But know I do. :wink:

I've tried your suggestion. As you can see, there is now an object named asterics with a property rest that provides a handle to all the functions of areCommunicator.js.

With webpack and npm, you can, however, already use import statements in source js scripts:

import { setBaseURI, ... } from "@asterics/are-rest";
/* or */
const { setBaseURI, ... } = require("@asterics/are-rest");
/* or */
const areRest = require("@asterics/are-rest");
...

The webpack bundle supports actually other module import/exports as well.

We simply need to agree to a name and make some additional test and we should be ready to go. However, there is still this CORS violation. Any thoughts on that?

klues commented 5 years ago

since we got stuck at the browser-generated request on opening an EventSource (SSE), which again breaks our applications in Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=1376310 ), the adaptions made in this PR do not really bring any advantage, so I'll close this PR.

klues commented 5 years ago

@sabicalija so in your library you should again use the original PUT requests instead of the POST requests.

asterics / AsTeRICS

Klaus/change rest methods to post #316