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

cfs:gridfs deprecated #3272

Closed
blaggacao opened this issue Sep 13, 2020 · 62 comments
Closed

cfs:gridfs deprecated #3272

blaggacao opened this issue Sep 13, 2020 · 62 comments

Comments

@blaggacao
Copy link
Contributor

blaggacao commented Sep 13, 2020

Issue

https://atmospherejs.com/cfs/gridfs

this blocks #3262 since because of an old mongodb version used as transitive dependency, it's impossible to connect to a remote mongo service (using some supported auth mecanism)

Despite #3270

What can we do?

In fact it looks like none of the cfs's packages is maintained any longer: https://atmospherejs.com/cfs

Affecting:

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

@blaggacao

Do you mean connecting to some specific remote MongoDB version?

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Do you mean this? #2977

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Maybe drop all those unmaintained packages and implement:

@xet7 With a little guidance, I might give it a try?

Do you mean connecting to some specific remote MongoDB version?

Yes it's mongo 4.2.6 — I'm using the mongodb operator which only supports SCRAM - which is only SCARM-SHA256 in case of 4.2.6.

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Do you mean this? #2977

Wekan never connect to mongodb saying member of replicaset can't be found

I had this error, but it seemed it was a mis-configuration in the URI, the persistent error (which might also be behind that reported issue) is that the auth methods don't match.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Is Meteor-Files same or different than ostrio-files? #3268 (comment)

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Is Meteor-Files same or different than ostrio-files? #3268 (comment)

It is the same! Thanks for the link — I didn't associate with your comment.

Still their recommendation about mongo-native GridFS-Bucket-Integration seems highly worth exploring.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

That ostrio-files changes are not merged yet, because those did not build yet on arm64 and s390x. But maybe that could be made working some way too.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

@urakagi and @blaggacao could look together how to get those ostrio-files changes as pull request to newest Wekan

@blaggacao
Copy link
Contributor Author

So you did do some work on https://github.com/wekan/wekan/tree/feature-ostrio-files — then you stopped because it didn't build — now it needs rebasing + fixing those build? +- right?

@blaggacao
Copy link
Contributor Author

feature-ostrio-files seems not to have any extra commits compared to master. Did I miss something?

@blaggacao
Copy link
Contributor Author

Ok, I'll hgive it a try to refactor the use of legacy packages towards ostrio:files & and gridfs-stream` towards https://github.com/VeliovGroup/Meteor-Files/wiki/GridFS-Bucket-Integration

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

And some could be at #3217

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Idea being, that migrating files from database to files or elsewhere, without overwriting duplicate files names, could somehow be made possible.

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Ok As far as I can judge and see some work is also in master:

0a1bfd3

Inline with https://github.com/VeliovGroup/Meteor-Files/wiki/GridFS-Bucket-Integration

Some work not directly related with this change is pending in #3217

Currently there are no commits in feature-ostrio-files which are not also currently in master. BUt master does not import ostrio:files — so this is where the delta lays.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Wow, that wiki page starts with 1. Create a GridFSBucket factory. Seems that some Java people coded that...

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

So this is from where it should start: https://github.com/wekan/wekan/pull/3273/files — now, lets make it work again 😜

@blaggacao
Copy link
Contributor Author

@xet7 might I disregard all ocurrences in fix-download-unicode/cfs_access-point.txt? — see: #3273 (comment)

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

@blaggacao

You can delete that directory fix-download-unicode and all related code. It was old fix #784 and it does not work anymore with newest Meteor.

@blaggacao
Copy link
Contributor Author

@xet7 It looks like FS.File is just a conventional file handle. Should they become collections?

models/trelloCreator.js:          const file = new FS.File();
models/wekanCreator.js:          const file = new FS.File();
client/lib/utils.js:    const file = new FS.File(fileObj)
client/components/cards/attachments.js:      const file = new FS.File(results.file);

@blaggacao
Copy link
Contributor Author

Are ES6 Imports ok?

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

About ES6, I think so ? https://github.com/wekan/wekan/blob/master/.eslintrc.json#L9

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Currently attachments are stored in database, and most have automated database backups

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Currently attachments are stored in database, and most have automated database backups

So all file handles should become MongoInternals.NpmModule.GridFSBucket. Sounds easy to do 👍

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

But in your code do not use Javascript arrow functions, because Python-based API documentation generation code does not like those arrow functions code.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Well, if there is Javascript arrow function, then Python code does not show any error, and just leaves out generating some part of documentation at https://wekan.github.io/api/

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Well, if there is Javascript arrow function, then Python code does not show any error,

Even better those factory and generator functions do not need to be exposed, I'll stow them away in a helper file in models/lib/*.js 👍 — less work, safer result

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

For serverside code, you could add check isServer. For clientside, isClient. Otherwise, code runs at both serverside and clientside.
https://github.com/wekan/wekan/blob/master/server/statistics.js#L3

Checking for logged in user with Meteor.user() and user being Admin Meteor.user().isAdmin
https://github.com/wekan/wekan/blob/master/server/statistics.js#L6

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Other roles like isWorker etc listed here https://github.com/wekan/wekan/wiki/REST-API-Role

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

BTW, for example when I added feature Assignee field like Jira, there this way I did prevent more than one assignee.

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Can local fs storage support be dropped? What is it good for? Would some follow up PR implementing feature par export from gridfs as replacement solve the use case?

I'd leaves to drop it, since it recreates the api and makes the code brittle to changes. there doesn't seem to be ootb support on ostrio:files... like if the package was trying to say: "why? who want's that?"

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

I think currently local fs storage does not work well enough. There is settings for it, but it depends does link to file really point to database or filesystem, is everything visible:
https://github.com/wekan/wekan/blob/master/docker-compose.yml#L239

@blaggacao
Copy link
Contributor Author

There is however storagePath?: string | ((fileObj: FileObj<MetadataType>) => string); in https://github.com/VeliovGroup/Meteor-Files/blob/master/docs/typescript-definitions.md and https://github.com/VeliovGroup/Meteor-Files/wiki/AWS-S3-Integration exemplifies as:

  const UserFiles = new FilesCollection({
    debug: false, // Change to `true` for debugging
    storagePath: 'assets/app/uploads/uploadedFiles',
...

I'll drop it in the meantime, but maybe once ostorio:files is working, it should be relatively easy to add back.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

If files are moved outside of database, then it's about how well linking to those files really works? Do those files have one-time view keys, are they at intranet, require Google login, or something else? Because private board attachments are not currently visible to non-logged in users, and it should still be that way.

@blaggacao
Copy link
Contributor Author

If files are moved outside of database, then it's about how well linking to those files really works? Do those files have one-time view keys, are they at intranet, require Google login, or something else? Because private board attachments are not currently visible to non-logged in users, and it should still be that way.

For such use cases https://github.com/VeliovGroup/Meteor-Files/wiki/AWS-S3-Integration seems like the right answer.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Yes. Currently it's first that with ostrio-files using attachments at database does still work, and then see does using ostrio-files solve your using remote database issue.

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Why is file type not preserved on non-image attachments?

    if (!fileObj.isImage()) {
      return {
        type: 'application/octet-stream',
      };
    }

Wouldn't it make more sense to preserve metadata?

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Yes.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

Currently problem is, that PDF files open also in slideshow, but then show nothing, because there is no PDF viewer setup.

Slideshow only shows JPG/JPEG/PNG/GIF bitmap image files, other file types should be excluded from slideshow, when those filetypes do not have a viewer that would work.

@blaggacao
Copy link
Contributor Author

Slideshow only shows JPG/JPEG/PNG/GIF bitmap image files, other file types should be excluded from slideshow, when those filetypes do not have a viewer that would work.

Sounds like a fix is appropriate over there at the slideshow implementation. We should take note to open a follow-up issue on this aspect of the refactoring.

@xet7
Copy link
Member

xet7 commented Sep 13, 2020

That AWS S3 integration seems to have some image processing Lambda possibilities for creating thumbnails:
https://github.com/VeliovGroup/Meteor-Files/wiki/AWS-S3-Integration#further-image-jpeg-png-processing-with-aws-lambda

Currently Wekan has some image resize code:
https://github.com/wekan/wekan/blob/master/docker-compose.yml#L248-L253

But currently that is optional, so all boards do load big images from big boards to browser and that slows down Wekan, because Wekan does not have thumbnails in general use.

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

Problem allow / deny:

These checks are run only when a client tries to write to the database directly, for example by calling update from inside an event handler. Server code is trusted and isn’t subject to allow and deny restrictions. That includes methods that are called with Meteor.call — they are expected to do their own access checking rather than relying on allow and deny.

It looks like the new implementation of allow / deny is without effects on on server side callers, was this the case before, too?

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 13, 2020

_ensureIndexis deprecated

Occurrences
models/triggers.js:    Triggers._collection._ensureIndex({ modifiedAt: -1 });
models/rules.js:    Rules._collection._ensureIndex({ modifiedAt: -1 });
models/invitationCodes.js:    InvitationCodes._collection._ensureIndex({ modifiedAt: -1 });
models/cardComments.js:    CardComments._collection._ensureIndex({ modifiedAt: -1 });
models/cardComments.js:    CardComments._collection._ensureIndex({ cardId: 1, createdAt: -1 });
models/attachments.js:    Attachments.files._ensureIndex({ cardId: 1 });
models/customFields.js:    CustomFields._collection._ensureIndex({ modifiedAt: -1 });
models/customFields.js:    CustomFields._collection._ensureIndex({ boardIds: 1 });
models/actions.js:    Actions._collection._ensureIndex({ modifiedAt: -1 });
models/swimlanes.js:    Swimlanes._collection._ensureIndex({ modifiedAt: -1 });
models/swimlanes.js:    Swimlanes._collection._ensureIndex({ boardId: 1 });
models/accountSettings.js:    AccountSettings._collection._ensureIndex({ modifiedAt: -1 });
models/checklistItems.js:    ChecklistItems._collection._ensureIndex({ modifiedAt: -1 });
models/checklistItems.js:    ChecklistItems._collection._ensureIndex({ checklistId: 1 });
models/checklistItems.js:    ChecklistItems._collection._ensureIndex({ cardId: 1 });
models/lists.js:    Lists._collection._ensureIndex({ modifiedAt: -1 });
models/lists.js:    Lists._collection._ensureIndex({ boardId: 1 });
models/orgUser.js:    OrgUser._collection._ensureIndex({ orgId: -1 });
models/orgUser.js:    OrgUser._collection._ensureIndex({ orgId: -1, userId: -1 });
models/cards.js:    Cards._collection._ensureIndex({ modifiedAt: -1 });
models/cards.js:    Cards._collection._ensureIndex({ boardId: 1, createdAt: -1 });
models/cards.js:    Cards._collection._ensureIndex({ parentId: 1 });
models/users.js:      Lists._collection._ensureIndex(value);
models/users.js:    Users._collection._ensureIndex({ modifiedAt: -1 });
models/users.js:    Users._collection._ensureIndex(
models/integrations.js:    Integrations._collection._ensureIndex({ modifiedAt: -1 });
models/integrations.js:    Integrations._collection._ensureIndex({ boardId: 1 });
models/org.js:    Org._collection._ensureIndex({ name: -1 });
models/announcements.js:    Announcements._collection._ensureIndex({ modifiedAt: -1 });
models/settings.js:    Settings._collection._ensureIndex({ modifiedAt: -1 });
models/boards.js:    Boards._collection._ensureIndex({ modifiedAt: -1 });
models/boards.js:    Boards._collection._ensureIndex(
models/boards.js:    Boards._collection._ensureIndex({ 'members.userId': 1 });
models/activities.js:    Activities._collection._ensureIndex({ createdAt: -1 });
models/activities.js:    Activities._collection._ensureIndex({ modifiedAt: -1 });
models/activities.js:    Activities._collection._ensureIndex({ cardId: 1, createdAt: -1 });
models/activities.js:    Activities._collection._ensureIndex({ boardId: 1, createdAt: -1 });
models/activities.js:    Activities._collection._ensureIndex(
models/activities.js:    Activities._collection._ensureIndex(
models/activities.js:    Activities._collection._ensureIndex(
models/checklists.js:    Checklists._collection._ensureIndex({ modifiedAt: -1 });
models/checklists.js:    Checklists._collection._ensureIndex({ cardId: 1, createdAt: 1 });
models/unsavedEdits.js:    UnsavedEditCollection._collection._ensureIndex({ modifiedAt: -1 });
models/unsavedEdits.js:    UnsavedEditCollection._collection._ensureIndex({ userId: 1 });

@blaggacao
Copy link
Contributor Author

@xet7 After kind of figuring out how this is supposed to work (took quite some time), I didn't find where to which object the url() method was attached in the previous code:

e4c28c3#diff-e37acd9674ee4d6c1221038429907195R107-R108

What was file.url() before?

@xet7
Copy link
Member

xet7 commented Sep 14, 2020

@blaggacao

Avatars are uploaded to database at Wekan user settings:

@xet7
Copy link
Member

xet7 commented Sep 14, 2020

@xet7
Copy link
Member

xet7 commented Sep 14, 2020

@xet7
Copy link
Member

xet7 commented Sep 14, 2020

In future, avatar can also be link to avatar image elsewhere, like at LDAP, Google Auth, Gravatar etc with image URL that is get in response to login to some auth system. In general, avatar URL points to database or elsewhere.

@blaggacao
Copy link
Contributor Author

blaggacao commented Sep 14, 2020

The PR is +- reviewable, now. I'm going to bed, now. Hope to catch up tomorrow with your comments. Thanks for this guidance. Felt like peer programming! Super nice!

Ps: I don't want to login to deepcode-ci-bot it asks me to send promotions to my email address. Looks a bit rogue for github automation tooling.

@xet7
Copy link
Member

xet7 commented Sep 14, 2020

@blaggacao

Thanks 👍 I just woke up ☕

@xet7
Copy link
Member

xet7 commented Sep 14, 2020

Well that deepcode sometime suggested Javascript arrow functions so it is not always right.

@razum2um
Copy link

just 2cents reg. "image processing Lambda possibilities for creating thumbnails": maybe approach with smart "resize-on-pull" proxy is the way to go, what do you think?

i.e. just adding settings-url like here: https://docs.mattermost.com/administration/image-proxy.html

@xet7
Copy link
Member

xet7 commented Oct 22, 2020

@razum2um

Wekan already has some code to make thumbnails. Wekan is used at private LANs that are not connected to Internet, so it's not possible to use Lambda there. Also, I think there is no need for separate proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants