Use typescript + cleanup data

[PierreSchwang]

Overview

Fixes #12

Dependency updates:
Closes #13
Closes #10
Closes #9
Closes #8
Closes #7
Closes #6
Closes #5
Closes #4
Closes #3
Closes #2

Description

Okay.

• Recode to use typescript instead of javascript.
• Use temporary files instead of keeping every file request in memory.
• Delete schematic files as well
• Extend prune:
  • Delete files in filesystem without matching record in database
  • Delete records from database instead of setting them to expired
  • Validate records have valid entry in filesystem
• Preserve original file name while also using random id as file name in storage

Maybe some other changes, but those should be the not as important I guess?

Major version increased as not compatible with older versions:

• Database column "expired" removed
• File Structure changed

but api endpoints kept the same.

Readme changes pending

Checklist

• Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
• Ensure that the pull request title represents the desired changelog entry.
• I tested my changes and approved their functionality.
• I ensured my changes do not break other parts of the code
• I read and followed the contribution guidelines

[PierreSchwang]

Oh, I forgot - example instance available @ https://arkitektonika.pschwang.eu/

[NotMyFault]

My work experience with TS/JS doesn't go beyond the bar minimum, therefore my review may or may not be a merge blocker.

[jacobsandersen]

Why is the expired db column removed? We use that to tell if a schematic once existed but has since been pruned, for user experience reasons. The HEAD request was used to check validity before sending the download request.

However, I don't know TS much so I can't really review this. And beyond that, it looks like if this is merged all of my code is essentially gone so I'm going to leave it up to @NotMyFault and the other IS members. If this merges, it's really not my project anymore and @PierreSchwang will have to take it over as SME.

[PierreSchwang]

Why is the expired db column removed? We use that to tell if a schematic once existed but has since been pruned, for user experience reasons.

Basically I understand the idea behind it, in the end I don't see any practical purpose in which this provides a benefit to a user. Basically, the user is only interested in: "Is my schematic there that I uploaded back then? No, good, then it was deleted. Yes, then I can continue to use it."
In that case, it would be a lot of dead data in the database, which you don't really need.

The HEAD request was used to check validity before sending the download request.

I personally couldn't see any real advantage in this either. After all, the GET or POST request says exactly the same in the end. I don't make a HEAD request if I can upload something without the intention to upload something anyway. Same for the download, and even if, you can intercept the status code without downloading the octet stream. But in favor of https://github.com/IntellectualSites/Arkitektonika-Client/blob/main/src/main/java/com/intellectualsites/arkitektonika/v1/ApiClient.java#L96 I would include the HEAD requests again.

However, I don't know TS much so I can't really review this. And beyond that, it looks like if this is merged all of my code is essentially gone

TS should simply bring standardization and type-safety to the project. The content is the same except for slight differences.

[NotMyFault]

@PierreSchwang Considering you already ditched some dependencies, do you mind running a yarn upgrade if it's not too much work? Renovate has risen lots of updates, some of them even covering CVEs.

[NotMyFault]

LGTM, thanks for your contribution. Though I think it would be nice to have GitHub actions tests here as well to test prs and commits as well. But that is something for a different pr/different time, as it ain't no release blocker for this one.

[PierreSchwang]

LGTM, thanks for your contribution. Though I think it would be nice to have GitHub actions tests here as well to test prs and commits as well. But that is something for a different pr/different time, as it ain't no release blocker for this one.

Thought about that as well - Maybe something is coming soon there

[jacobsandersen]

Why is the expired db column removed? We use that to tell if a schematic once existed but has since been pruned, for user experience reasons.

Basically I understand the idea behind it, in the end I don't see any practical purpose in which this provides a benefit to a user. Basically, the user is only interested in: "Is my schematic there that I uploaded back then? No, good, then it was deleted. Yes, then I can continue to use it." In that case, it would be a lot of dead data in the database, which you don't really need.

The HEAD request was used to check validity before sending the download request.

I personally couldn't see any real advantage in this either. After all, the GET or POST request says exactly the same in the end. I don't make a HEAD request if I can upload something without the intention to upload something anyway. Same for the download, and even if, you can intercept the status code without downloading the octet stream. But in favor of https://github.com/IntellectualSites/Arkitektonika-Client/blob/main/src/main/java/com/intellectualsites/arkitektonika/v1/ApiClient.java#L96 I would include the HEAD requests again.

However, I don't know TS much so I can't really review this. And beyond that, it looks like if this is merged all of my code is essentially gone

TS should simply bring standardization and type-safety to the project. The content is the same except for slight differences.

It's fine if you want to remove the expire column or HEAD requests. Just be aware that the Java API will need to be updated and released first, so the PR is blocked until then. The Java API used the HEAD requests to know if a schematic once existed but had since been deleted (file gone from disk, but DB marked as expired.) This was an original requirement from City, and that is why it was there. If those requirements have changed, I really don't care then. Just make sure the Java API matches what Arkitektonika provides.

As for TS, I don't have time to learn it and therefore I will not be learning it so that I can remain responsible for Arkitektonika when this merges. Since this is a major change to a dialect that I do not have a strong footing with, when this merges I will no longer be responsible for the project. That will fall to you, since you are making the changes.

If you're not okay with that, then the PR should probably remain blocked until a suitable replacement is found. I am far too busy to uproot my other obligations just to learn TS for this, even if the differences are minimal as you say. I see many things in the diff that I do not currently understand.

[NotMyFault]

It's fine if you want to remove the expire column or HEAD requests. Just be aware that the Java API will need to be updated and released first, so the PR is blocked until then. The Java API used the HEAD requests to know if a schematic once existed but had since been deleted (file gone from disk, but DB marked as expired.) This was an original requirement from City, and that is why it was there. If those requirements have changed, I really don't care then. Just make sure the Java API matches what Arkitektonika provides.

That is a valid concern. I'm not sure where or when we use the API to determine whether a schematic has been deleted or not, maybe we can keep it up if it's actually useful or remove it if we never utilized it in the first place.

[PierreSchwang]

The only known utilization iirc was the enum in the arkitektonika client itself. I've never seen a use case where especially expired was checked.

[NotMyFault]

Then we may can remove it, if we don't actually need it.

[PierreSchwang]

Then we may can remove it, if we don't actually need it.

https://github.com/IntellectualSites/Arkitektonika-Client/blob/main/src/main/java/com/intellectualsites/arkitektonika/ResourceStatus.java#L37 should be marked as deprecated then - when and if this PR is merged

[jacobsandersen]

https://github.com/IntellectualSites/Arkitektonika-Client/blob/main/src/main/java/com/intellectualsites/arkitektonika/v1/ApiClient.java#L96

Head request usage and checking existence (410 response)

https://github.com/IntellectualSites/SchematicWeb/blob/master/mixin/schematic-workflow-mixin.js#L31

SchematicWeb checks the head response before continuing:

• https://github.com/IntellectualSites/SchematicWeb/blob/master/components/download/DownloadInterface.vue#L19
• https://github.com/IntellectualSites/SchematicWeb/blob/master/components/delete/DeleteInterface.vue#L19

As it currently stands this will break the Java API and SchematicWeb. I cannot speak to the Java API but SchematicWeb uses the head requests to provide a better user experience. Letting the user know that it knows about the key they provided but that the file was deleted is a better user experience than it telling the user that it has no idea what that key is. It removes a source of confusion, namely that the user can be assured they have a valid key and that the file is simply gone. That was the original intention.

If this is seen as bloat and no longer necessary, it's fine by me. But these will need to be resolved first. I do not currently have time to make these changes in SchematicWeb because I am busy with school. Someone else can submit the PR to process these removals and update the frontend's workflow to handle the deleted HEAD requests. I should be able to review.

There is certainly more utilization than seems to be understood, so there should be more research before blindly removing things.

[NotMyFault]

Letting the user know that it knows about the key they provided but that the file was deleted is a better user experience than it telling the user that it has no idea what that key is. It removes a source of confusion, namely that the user can be assured they have a valid key and that the file is simply gone. That was the original intention.

Good to know, I wasn't aware of that aspect. In my opinion, we should probably keep it for a better UX, considering it was indeed already used before, if that would be possible @PierreSchwang ?

[PierreSchwang]

Letting the user know that it knows about the key they provided but that the file was deleted is a better user experience than it telling the user that it has no idea what that key is. It removes a source of confusion, namely that the user can be assured they have a valid key and that the file is simply gone. That was the original intention.

Good to know, I wasn't aware of that aspect. In my opinion, we should probably keep it for a better UX, considering it was indeed already used before, if that would be possible @PierreSchwang ?

Yup, that's a good reason indeed - Gonna re-add that again

[NotMyFault]

Yup, that's a good reason indeed - Gonna re-add that again

After that, I guess we are good to pull this PR. The concerns have been outlined and addressed, no need to keep it on hold longer.

[NotMyFault]

@PierreSchwang is this PR ready for merge?

[PierreSchwang]

@PierreSchwang is this PR ready for merge?

Should be, yup - I haven't thought about migrating existing schematics - that's most likely not possible - is that bad?

[NotMyFault]

What do you mean by migrating existing schematics?

[PierreSchwang]

What do you mean by migrating existing schematics?

When updating Arkitektonika to this changes, the old structure is most likely not compatible anymore, as many things changed. therefor uploaded schematics are most likely unusable.

[NotMyFault]

Oic, this likely affects all instances (assuming people run standalone instances) then? But our instance does serve schematics for 30 days, after that they expire anyway.

[NotMyFault]

@PierreSchwang looks like there are needed dependencies neither the docker image nor plain npm installs.

[PierreSchwang]

@PierreSchwang looks like there are needed dependencies neither the docker image nor plain npm installs.

docker build should be called. npm install requires python3 for node-gyp

[NotMyFault]

If nobody else from @IntellectualSites/admins wants to take a further look, I would go ahead to merge the PR and deploy it to our org dockerhub, to go live with it.

[PierreSchwang]

I wanted to implement some kind of migration for old data - I've no idea yet when I got the time for that tho

[thomasmny]

When using this branch to upload a ~501x501 schematic that goes from y=2 to y=314 (aka a shit ton of blocks), I still receive a 400 error

[PierreSchwang]

When using this branch to upload a ~501x501 schematic that goes from y=2 to y=314 (aka a shit ton of blocks), I still receive a 400 error

@Trichtern A bit late, but could you send me the schematic file for testing purposes? If the schematic should not be shared publicy, feel free to send via Discord (Pierre#9657) - Thanks!

[thomasmny]

I think it was this schematic

plot.schem.zip

[NotMyFault]

Pierre die a great job on the rewrite. We just confirmed it works with PlotSquared without further modifications of the Java implementation.
If a @IntellectualSites/core-team member would like to review the PR, please go ahead or request yourself as reviewer.
Otherwise I'd say we pull the PR in anytime soon once Pierre is happy with it.