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

Fix certain library files being only half-deleted #69

Closed
wants to merge 1 commit into from
Closed

Fix certain library files being only half-deleted #69

wants to merge 1 commit into from

Conversation

alpatron
Copy link
Contributor

This PR fixes (or tries to fix) issue #68, which I raised.

What I did is I took the file-deletion code from an endpoint that didn't have any problems, like charset,

@api.marshal_with(simpleResponse)
def delete(self, id):
"""
Deletes charset.
"""
charset = FcCharset.query.filter(FcCharset.id == id).one()
charset.deleted = True
db.session.commit()
path = os.path.join(CHARSET_DIR, charset.path)
if os.path.exists(path):
os.remove(path)
return {
'status': True,
'message': 'Charset sucesfully deleted.'
}, 200

and copied it to the two endpoints that do have the issue (Markov files and mask sets). The same or similar file-deletion code is present also in other library endpoints, so I think the issue is just that the file-deletion code was forgotten to be pasted in.

I tested the fix and it seems to have resolved the issue, at least to my knowledge.

@alpatron alpatron changed the title Fix library files being only half-deleted Fix certain library files being only half-deleted Feb 27, 2023
@davidbolvansky
Copy link
Contributor

But if you delete it and you have some existing jobs with that markov and somebody wants to open it from job detail, something bad will happen, possibly just empty text view?

So sometime ago we decided that we should not remove files, just introduce new deleted flag.

I am aware of your other bug reports but not sure how we want to fix it... we can name uploaded file that is saved on the server as we want, so just maybe generate random filename? Kinda edge case..

@alpatron
Copy link
Contributor Author

alpatron commented Mar 1, 2023

But if you delete it and you have some existing jobs with that markov and somebody wants to open it from job detail, something bad will happen, possibly just empty text view?

So sometime ago we decided that we should not remove files, just introduce new deleted flag.

I am aware of your other bug reports but not sure how we want to fix it... we can name uploaded file that is saved on the server as we want, so just maybe generate random filename? Kinda edge case..

@davidbolvansky
Yes, I thought a bit about that, about files being deleted but required by existing jobs. In that case, however, let me ask you a question: dictionaries, PCFGs, rules, and charsets have their files deleted upon deleting them in Webadmin, does this not cause the same potential problem as with deleting Markov files? If that is the cause, I suppose the expected behaviour is that the upload of previously-used files fails in some way (or that the files are stored using unique names), and I suppose that the behaviour of Webadmin deleting the files of dictionaries, PCFGs, rules, and charsets is actually unwanted.

Python has a built-in uuid library that could be used for the random filename approach.

@ihranicky
Copy link
Member

As @davidbolvansky mentioned, we cannot just delete or edit files, as they can be dependencies of existing jobs. Possible solutions are:
a) Check if the file is not mapped to a job or
b) Generate unique filenames for each added library element (e.g. using uuid libary as @alpatron suggests).
I would go with b), but this requires of all library endpoints and editing of default DB content and default example library collection.

@ihranicky ihranicky closed this Apr 7, 2023
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