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

Refactoring: Prefix URLs pointing to the backend #296

Closed
roschaefer opened this issue Feb 5, 2019 · 19 comments

Comments

@roschaefer
Copy link
Contributor

commented Feb 5, 2019

Describe the bug
The server side rendered frontend serves our images at /api/uploads. All requests to /api are proxied to the backend, which serves images at /uploads/. Right now, the backend mutates all URLs in the response to be prefixed with /api. This is a design flaw as the backend knows how the frontend proxies requests.

Expected behavior
We should prefix all <img src="URL"> urls in the frontend with /api so the backend does not need to do it for the frontend.

Screenshots
2019-02-05-094935_1920x1080_scrot

@roschaefer roschaefer changed the title Refactoring: Prefix all image tag src attributes with `/api/` Refactoring: Prefix all <img> tag src attributes with `/api/` Feb 5, 2019

@datenbrei datenbrei changed the title Refactoring: Prefix all <img> tag src attributes with `/api/` Refactoring: Prefix some <img> tag src attributes with `/api/` Feb 5, 2019

@datenbrei datenbrei changed the title Refactoring: Prefix some <img> tag src attributes with `/api/` Refactoring: Prefix URLs pointing to the backend Feb 5, 2019

@tansaku

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@roschaefer having a look at this issue - in the seeded data all the images associated with posts are from the https://lorempixel.com site (presumably from the faker module) - we couldn't find any in the seeded data that have urls like /api/uploads, or /uploads or anything.

Looking at the alpha system we did see URLs like this:

https://img-alpha.human-connection.org/3wFqNObuo-R9WQINClzV-8Cw17g=/243x100/smart/filters:blur(30):background_color(fff)/https://api-alpha.human-connection.org/uploads/teaserImg_59970fe0-3f95-11e9-b57e-39c2729f39e9.jpeg

Is this issue not one that specifically concerns the situation when a user uploads a new image through the system? that functionality is not yet present in the new system, so difficult for us to test perhaps?

Perhaps this is something we could examine more easily if we were importing the alpha data into the new system?

@appinteractive

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@tansaku did you take a look 👀 on the badges? They are coming from the api.

@tansaku

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@appinteractive we did not - the example shows image posts so that's where we focused our attention, but that's a good tip - thanks :-)

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@tansaku the image paths of the seeds in the backend are fine BUT on their way out, the backend middleware adds a prefix /api/ to all strings that look like a url. That's the problem the code smell (violation of the law of demeter).

Here's the middleware:
https://github.com/Human-Connection/Nitro-Backend/blob/master/src/middleware/fixImageUrlsMiddleware.js

Here's the test:
https://github.com/Human-Connection/Nitro-Backend/blob/master/src/middleware/fixImageUrlsMiddleware.spec.js#L14

The returned string of the backend should be the same except the unnecessary prefix.

The responsibility of the frontend is to add the prefix /api to all image tags that get their url from the backend. The frontend knows the endpoint that is proxied to the backend here thus it should be responsible for adding these prefixes.

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Literally every string that contains one of the legacy urls is substituted with /api. It should be substituted with empty string.

@tansaku

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

thanks @roschaefer - I think we understand all that - the problem is that we don't see any urls of that form when we run the system locally, and we can't upload images to test. The issue is we want to be able to see that the fix we make has actually worked. Did you upload the original screenshot? Was that with imported data?

Anyway, perhaps we can do as @appinteractive suggests and look at badges to find an example of the rewriting taking place ...

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Ah! I'm so driven by test-driven development that I forgot one might also want to try out the software. Excuse my ignorance, yes, I confirm @appinteractive that the only occurence I know of are the .svg files of the badges.

Just double-checked and there seems to be more images.
2019-03-18-233030_1920x1080_scrot

You can see the badges coming from the api. You can find the .svg files in the /uploads folder.

@tansaku do you like getting distracted? More problems:

  • The /uploads folder does not get deployed along with the backend. Therefore we get 404 for every badge on all our kubernetes deployments.
  • The backend cannot be scaled horizontally. If backend reads files from disk, e.g. the /uploads folder, then we need a kubernetes volume. Digital Ocean does not support multi-stage access on volumes. Hence, only one pod can claim a persistent volume at a time and we're not able to scale the backend. Solution: Don't let the backend do the /uploads.

@roschaefer roschaefer transferred this issue from Human-Connection/Nitro-Web Mar 26, 2019

@roschaefer roschaefer added this to the Gaia milestone Mar 26, 2019

@ulfgebhardt ulfgebhardt added this to To Do in Human-Connection via automation Apr 1, 2019

@roschaefer roschaefer removed their assignment Apr 1, 2019

@appinteractive appinteractive modified the milestones: Sprint: Gaia, Hera Apr 2, 2019

@aonomike

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

<img title="indiegogo_en_turtle" src="/img/badges/indiegogo_en_turtle.svg" class="hc-badge">
@roschaefer @appinteractive we looked at the badges and they are not being loaded from the uploads folder. All the badges are in webapp/static/img/badges and the backend does not seem to be involved.

@aonomike

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Screen Shot 2019-04-04 at 7 25 51 PM

Other images come from amazon and there is no `/uploads` directory seen
@tansaku

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@roschaefer @appinteractive looked at this together with @aonomike - we can see the backend/src/middleware/fixImageUrlsMiddleware that is doing the rewriting on the back end but we can find no mention of it on the frontend in the current system. We understand that maybe you'd like the rewriting to take place on the frontend, but is the /api/upload url something we will only see from legacy data from the old system?

or will those URLs be created when image uploading is enabled on the new frontend?

@tansaku

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@ulfgebhardt @appinteractive we just heard from @mattwr18 that this ticket got de-prioritized. I had written this blog post outlining some approaches that @aonomike and I were taking

https://nonprofits.agileventures.org/2019/03/28/pairing-with-mike-on-human-connection-vuejs/

We understand that it's low priority and perhaps difficult to explain, but I think Mike and I were on the cusp of understanding it; so if you do have a chance to review and answer our questions we'd very much appreciate it :-)

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@tansaku @aonomike this ticket is still part of our ongoing milestone Inanna so I don't think it got de-prioritized.

In general. all volunteers are free to choose whatever task they like. I would prefer to have everyone productive over having everyone working on the important tasks. Plus, if there's a good-first-issue I try to keep that open for new developers. 🙂

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I will have a look, if anybody wants to pair: https://hangouts.google.com/hangouts/_/t7c5bc33cjburi5vbmaxg5w67ye

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

2019-04-23-175825_1920x1080_scrot

The backend is capable of serving /public/ I will now have a look why the frontend does not serve the assets from the backend.

@roschaefer roschaefer referenced this issue Apr 23, 2019

Merged

296 image component #499

1 of 5 tasks complete
@appinteractive

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@roschaefer Just a comment on the upload on the Frontend side: it’s not a good idea from my perspective to separate the data and do some part on the Frontend and some part on the backend, this will explode if we want to serve images for activity pub for example or other aspects. Separating that seems to be a really bad idea from my point of view. At least the Frontend should really not be responsible for that.

Maybe a microservice for that job would be a solution, acting like a cdn.

@Lulalaby

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@roschaefer Just a comment on the upload on the Frontend side: it’s not a good idea from my perspective to separate the data and do some part on the Frontend and some part on the backend, this will explode if we want to serve images for activity pub for example or other aspects. Separating that seems to be a really bad idea from my point of view. At least the Frontend should really not be responsible for that.

Maybe a microservice for that job would be a solution, acting like a cdn.

I agree to you

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@appinteractive @Lulalaby I believe that #499 is going into this direction, isn't it? The frontend does not need to save images for badges, that are stored in the database + the frontend knows that it is proxying to /api so it's the responsibility of the frontend to prefix images with /api.

Did I understood your comment correctly, @appinteractive ?

@ulfgebhardt

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@roschaefer is this fixed?

@roschaefer

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@ulfgebhardt nope, it's closed once #499 gets merged

Human-Connection automation moved this from To Do to Done May 10, 2019

@ulfgebhardt ulfgebhardt referenced this issue May 27, 2019

Open

🌟 [EPIC] DevOps / Other #427

28 of 33 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.