Include the Monarch link app as a service on IMAGE-server #894

VenissaCarolQuadros · 2024-10-08T18:37:13Z

This PR merges a containerized version of the Monarch web application that links the IMAGE-TactileAuthoring tool to the Monarch client.

As per discussions, this application has been made into a service on the IMAGE-server. New channels can be created and existing channels can be updated by the authoring tool by posting to https://monarch.unicorn.cim.mcgill.ca/create/<subscribed_code>. The JSON can be accessed by the Monarch client by getting from https://monarch.unicorn.cim.mcgill.ca/display/<subscribed_code>. Resolves #888.

Testing:

Checked that restarting container clears previously created channel
Checked that a new channel can be created using publish on the authoring tool and getting the same on the Monarch

<svg width="254.00000000000003" height="105.83333" xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg" version="1.1" xml:space="preserve">
<g class="layer" data-image-layer="Layer 1">
<ellipse cx="59.5" cy="27.82" fill="#FF0000" id="svg_1" rx="27" ry="25" stroke="#000000" stroke-width="5"/>
</g>
</svg>

was published on code 123456.

Checked that a channel can be updated when the code and secret id is correct
Graphic on 123456 was updated to

<svg width="254.00000000000003" height="105.83333" xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg" version="1.1" xml:space="preserve">
 <g class="layer" data-image-layer="Layer 1">
  <rect fill="#FF0000" height="34.62" id="svg_2" stroke="#000000" stroke-width="5" width="84.83" x="26.47" y="33.31"/>
 </g>
</svg>

and the change was observed on the Monarch

Checked that update fails when the secret id is incorrect
The update failed when the secret key was changed

Required Information

I referenced the issue addressed in this PR.
I described the changes made and how these address the issue.
I described how I tested these changes.

Coding/Commit Requirements

I followed applicable coding standards where appropriate (e.g., PEP8)
I have not committed any models or other large files.

New Component Checklist (mandatory for new microservices)

I added an entry to docker-compose.yml and build.yml.
I created A CI workflow under .github/workflows.
I have created a README.md file that describes what the component does and what it depends on (other microservices, ML models, etc.).

OR

I have not added a new component in this PR.

…uded license in app

JRegimbal

Comments within the file, plus the inputs are not checked. I was able to cause an internal server error by omitting or incorrectly entering fields in the POST requests.

.github/workflows/monarch-link-app-service.yml

JRegimbal · 2024-10-10T15:45:14Z

services/monarch-link-app/Dockerfile

@@ -0,0 +1,19 @@
+FROM python:3.10


The current versions is 3.13 - why are you using 3.10?

JRegimbal · 2024-10-10T15:52:16Z

services/monarch-link-app/app.py

+def write_data(svgData):
+    with open("data.json", "w") as outfile:
+        json.dump(svgData, outfile)
+
+
+def read_data():
+    with open('data.json', 'r') as openfile:
+        try:
+            return json.load(openfile)
+        except Exception:
+            return dict()


OK for quick prototyping, but this should not be used in production. Would suggest something like sqlite3 that may scale better.

Created #897 to keep track of this

JRegimbal · 2024-10-10T15:52:30Z

services/monarch-link-app/app.py

+            return dict()
+
+
+@app.route("/create/<id>", methods=["POST"])


Are there restrictions on what "id" can be here?

Also - any reason why the create and update methods were combined?

I haven't limited this currently but I typically use a 6-digit numeric code. Flask has options to limit this something generic like 'int' or we can also build custom 'converters' if we want to enforce this.

EDIT: I have added in a custom converter.

JRegimbal · 2024-10-10T15:54:59Z

services/monarch-link-app/app.py

+        req_data = request.get_json()
+        svgData = read_data()
+        if id in svgData:
+            if (svgData[id])["secret"] == req_data["secret"]:


Generally considered bad practice to save this in plain text...compare using something like bcrypt?

JRegimbal · 2024-10-10T15:56:13Z

services/monarch-link-app/app.py

+                write_data(svgData)
+                return jsonify("Graphic in channel "+id+" has been updated!")
+            else:
+                return jsonify("Unauthorized access to existing channel!")


Unauthorized access should return 401 code.

JRegimbal · 2024-10-10T15:57:49Z

services/monarch-link-app/app.py

+                           "data": req_data["data"],
+                           "layer": req_data["layer"]}
+            write_data(svgData)
+            return jsonify("New channel created with code "+id)


In the model we discussed earlier, the ID and secret was generated on the server and returned to the client. Create only allowed the client to specify a title. Any reason for the changes?

Oops, it looks like I've somehow completely messed up the implementation here! :')
Will probably need to touch base in person to sort this out.

JRegimbal · 2024-10-10T15:58:06Z

services/monarch-link-app/app.py

+            return jsonify("New channel created with code "+id)
+
+
+@app.route("/display/<id>", methods=["GET"])


Any reason for the change in name from receive? Also should check the ID.

JRegimbal · 2024-10-10T16:04:54Z

services/monarch-link-app/requirements.txt

+click==8.1.3
+colorama==0.4.6
+Flask==2.2.2
+Flask-Cors==4.0.2
+Flask-Login==0.6.2
+Flask-SQLAlchemy==3.0.2
+greenlet==2.0.1
+itsdangerous==2.1.2
+Jinja2==3.1.2
+MarkupSafe==2.1.1
+six==1.16.0
+SQLAlchemy==1.4.43
+SQLAlchemy-Utils==0.38.3
+Werkzeug==2.2.2
+gunicorn==22.0.0


Suggest setting only what you use directly with semver and allowing some newer verisons. pip freeze output is almost always overly restrictive.

JRegimbal · 2024-10-10T16:10:00Z

services/monarch-link-app/app.py

+            response.add_etag(hashlib.md5(
+                (svgData[id]["data"]+svgData[id]["layer"]).encode()))
+            response.make_conditional(request)


Co-authored-by: Juliette Regimbal <[email protected]>

VenissaCarolQuadros · 2024-10-18T19:04:09Z

I ran additional tests with POST requests with incorrect/ omitted fields. It now returns 400 in these cases.

JRegimbal · 2024-10-21T12:48:37Z

services/monarch-link-app/app.py

+    while code in svgData:
+        code = generate_code(svgData)


Putting a pin in this for later updates.

JRegimbal · 2024-10-21T12:50:23Z

services/monarch-link-app/app.py

-@app.route("/create/<id>", methods=["POST"])
+# generate an id that does not already exist
+def generate_code(svgData):
+    code = ''.join([str(random.randint(1, 9)) for i in range(6)])


It looks like randint(a, b) returns a value n where a <= n <= b. This conflicts with the check for values between 1 and 8 on line 71.

https://docs.python.org/3/library/random.html

Oopsies! I referred numpy randint docs 😶

Ah yes, the legally distinct randint. No worries!

JRegimbal · 2024-10-21T12:51:19Z

services/monarch-link-app/app.py

+        # has six digits between 1 and 8
+        pattern = re.compile("[1-8]{6}")
+        # also has a length of six
+        if not pattern.match(value) or len(value) != 6:


Suggest updating the pattern to include line start/end (^[1-8]{6}$) to avoid the separate length check.

JRegimbal · 2024-10-21T12:52:04Z

services/monarch-link-app/app.py

+app.url_map.converters['code'] = CodeConverter
+
+
+@app.route("/create/<string:title>", methods=["POST"])


I wouldn't have put the title as part of the path here. This makes more sense as part of the request body.

VenissaCarolQuadros · 2024-10-21T20:09:05Z

Made the requested changes.
I also noticed that I was not saving 'title' to JSON- I've included it and also made it possible to change this when 'update/' is called.

Also, an exception was being raised when 'display/' was called on a valid ID that didn't exist in the JSON since abort() raises an exception.

JRegimbal

Looks good! We should chat tomorrow during the meeting about merging this.

jeffbl · 2024-12-09T21:07:17Z

Marking for deploy on pegasus since I haven't noticed any ill effects on unicorn. @JRegimbal @VenissaCarolQuadros please push back if you think this could cause issues.

VenissaCarolQuadros · 2024-12-09T21:50:22Z

Unless there is already some cleanup happening on Pegasus, we need have a way to clear the .json file this app creates since this could bulk up over time. (It is cleared on Unicorn every time the container is rebuilt)

jeffbl · 2024-12-09T21:54:44Z

Is there a work item for doing this? (If so, please link here, otw it sounds like we should create one?)

And I assume deploying this is also blocked on Shared-Reality-Lab/IMAGE-website#73?

VenissaCarolQuadros added 2 commits October 3, 2024 14:21

Initial commit of monarck link app

be252ea

Fixes to get Monarch link app working

cb5662d

VenissaCarolQuadros self-assigned this Oct 8, 2024

VenissaCarolQuadros added 4 commits October 8, 2024 15:54

Included entry in docker-compose, build, created CI workflow and incl…

8712cc3

…uded license in app

PEP8 fixes

02e0938

PEP8 fixes

4d2df3b

Remove build from docker-compose and fill in README

4fc1fd7

VenissaCarolQuadros marked this pull request as ready for review October 9, 2024 15:40

VenissaCarolQuadros requested a review from JRegimbal October 9, 2024 15:41

JRegimbal requested changes Oct 10, 2024

View reviewed changes

VenissaCarolQuadros and others added 7 commits October 10, 2024 15:21

Update .github/workflows/monarch-link-app-service.yml

8ca2654

Co-authored-by: Juliette Regimbal <[email protected]>

Fix github workflow entry

93c8c83

Updated python version and modified requirements to be less restrictive

45ca701

Split create and update paths and use bcrypt for secret key

46b546d

PEP8 fixes

052484f

Add comments, deal with invalid inputs

d882c21

PEP8 fixes

c5e6763

VenissaCarolQuadros assigned JRegimbal and unassigned VenissaCarolQuadros Oct 18, 2024

JRegimbal approved these changes Oct 21, 2024

View reviewed changes

JRegimbal assigned VenissaCarolQuadros and unassigned JRegimbal Oct 21, 2024

Fix code generation and check, and save title in JSON

6f5de0a

VenissaCarolQuadros assigned JRegimbal and unassigned VenissaCarolQuadros Oct 21, 2024

JRegimbal approved these changes Oct 22, 2024

View reviewed changes

Move unicorn specific config to test-docker-compose

3625fac

JRegimbal approved these changes Oct 23, 2024

View reviewed changes

Merge branch 'main' into monarch-link-app

b8a81c4

JRegimbal merged commit d1bfea8 into main Oct 23, 2024
2 checks passed

JRegimbal deleted the monarch-link-app branch October 23, 2024 15:53

jeffbl added the deploy! deploy into production from test! label Dec 9, 2024

jeffbl removed the deploy! deploy into production from test! label Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include the Monarch link app as a service on IMAGE-server #894

Include the Monarch link app as a service on IMAGE-server #894

VenissaCarolQuadros commented Oct 8, 2024 •

edited

Loading

JRegimbal left a comment

JRegimbal Oct 10, 2024

JRegimbal Oct 10, 2024

VenissaCarolQuadros Oct 17, 2024

JRegimbal Oct 10, 2024

JRegimbal Oct 10, 2024

VenissaCarolQuadros Oct 10, 2024 •

edited

Loading

JRegimbal Oct 10, 2024

JRegimbal Oct 10, 2024

JRegimbal Oct 10, 2024

VenissaCarolQuadros Oct 10, 2024

JRegimbal Oct 10, 2024

JRegimbal Oct 10, 2024

JRegimbal Oct 10, 2024

VenissaCarolQuadros commented Oct 18, 2024

JRegimbal Oct 21, 2024

JRegimbal Oct 21, 2024

VenissaCarolQuadros Oct 21, 2024

JRegimbal Oct 22, 2024

JRegimbal Oct 21, 2024

JRegimbal Oct 21, 2024

VenissaCarolQuadros commented Oct 21, 2024

JRegimbal left a comment •

edited

Loading

jeffbl commented Dec 9, 2024

VenissaCarolQuadros commented Dec 9, 2024

jeffbl commented Dec 9, 2024

		return jsonify("New channel created with code "+id)


		@app.route("/display/<id>", methods=["GET"])

		app.url_map.converters['code'] = CodeConverter


		@app.route("/create/<string:title>", methods=["POST"])

Include the Monarch link app as a service on IMAGE-server #894

Include the Monarch link app as a service on IMAGE-server #894

Conversation

VenissaCarolQuadros commented Oct 8, 2024 • edited Loading

Required Information

Coding/Commit Requirements

New Component Checklist (mandatory for new microservices)

JRegimbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenissaCarolQuadros Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenissaCarolQuadros commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenissaCarolQuadros commented Oct 21, 2024

JRegimbal left a comment • edited Loading

Choose a reason for hiding this comment

jeffbl commented Dec 9, 2024

VenissaCarolQuadros commented Dec 9, 2024

jeffbl commented Dec 9, 2024

VenissaCarolQuadros commented Oct 8, 2024 •

edited

Loading

VenissaCarolQuadros Oct 10, 2024 •

edited

Loading

JRegimbal left a comment •

edited

Loading