Api restructure #125

ahmednfwela · 2021-11-05T04:07:16Z

This PR is to restructure the API under the following targets

a DBContext targets a single database
a MongoContext targets a MongoDB connection which allows accessing multiple DBContexts (multiple databases)
the static api is to be routed through a singleton MongoContext with a singleton DBContext for convenience to use when no DI containers are being used
MongoContexts and DBContexts should be cheap to create, cachable and configurable
MongoContext will store connection-level mappings and convention, while DBContext will store Database-level mappings and conventions

solves #124
solves #123
solves #126

dj-nitehawk · 2021-11-05T09:43:22Z

@ahmednfwela we can drop support for netstandard2.0 and bring it up to 2.1 if it's gonna be of some help.

also pls keep the performance benchmarks in mind as well. we don't wann loose too much when compared against the official driver.

and have you had a chance to see if relationships can be supported with your restructure/multi-tenancy?

also if there's anything i can do to help, pls let me know.

good luck brotha!

ahmednfwela · 2021-11-05T10:28:07Z

I don't think upgrading the TargetFramework will be necessary, I was just testing something with 2.1.
As for the performance, it would automatically get better since we are removing the overhead of maintaining a reference to all the databases.

For relationships, here is what we could do:
The current api handles the relationships using Many/One objects, and they need to get a reference to IMongoCollection and this is where the problem happens.
To get a reference to an IMongoCollection you need the DBContext, which means you need to somehow tell the model object about the DBContext, this was easy when the static DB class was used, but not anymore.

To solve this we have 2 ways

Pass the DBContext to the model
Change Many/One so that they don't depend on IMongoCollection

In my opinion the second option is better, since it's less intrusive and more maintainable, but it will break the existing api since it depends strictly on IMongoCollection.

I will push some commits with what I think should be the right pattern to use, and see if it's ok.

dj-nitehawk · 2021-11-05T11:36:18Z

yeah no worries. let's break the many/one api if need be.
if all pans out ok, we can release it as a major version jump with breaking changes. i'm sure ppl would understand.
it would future-proof a lot of things.

ahmednfwela · 2021-11-05T12:31:53Z

@dj-nitehawk please check the latest commit, I split Find into 2 classes

FindBase<T, TProjection, TSelf> is only responsible for building the query, and doesn't care about what collection it runs on.
Find<T, TProjection> takes query information from FindBase and adds DBContext and IMongoCollection on top of it, which makes the query executable.

if you like this pattern, we can generalize it for the other classes too
and it won't break the public API, since all the constructors are internal

dj-nitehawk · 2021-11-05T14:06:30Z

yeah looks good... go for it 👍

* Major refactoring * No more repeating code in builders * Multi-tenancy is per database now instead of per collection, which should be cleaner

ahmednfwela · 2021-11-05T15:16:41Z

@dj-nitehawk I refactored the builders now, and they look way cleaner than before

P. S. also enabled nullable in the entire project

dj-nitehawk · 2021-11-05T15:27:35Z

yes i've been following your work. looks awesome!!!
and yeah i was gonna enable nullable myself. you beat me to it ;-)
good job so far man!!!
looking forward to testing this...

bobbyangers · 2021-11-05T19:47:39Z

@ahmednfwela I am reviewing your work, it's moving along quite well.

It seems we have many common coding style.

Note1: (Maybe) Instead of MongoContext, consider renaming to ServerContext; it is more general

Note2: I think we don't need to "hardcode" the notion of tenantId into the database naming; and leave that logic to the Dependency injection that way we don't need to "force" anyone to use a specific pattern for naming the DBs.
For example, we might use a pattern similar to Finbuckle.Multinant and store the tenantinfo/db mapping in a tenant database. (i'll write the extension, once you are done with this pass)

ahmednfwela · 2021-11-05T21:48:06Z

@bobbyangers yes, I think I will remove the tenant id altogether and let the tenancy handling out of the Context
and i renamed MongoContext to MongoServerContext

bobbyangers · 2021-11-06T00:38:24Z

@ahmednfwela, it's looking better and better.

if I can do something to help, let me know.

ahmednfwela · 2021-11-06T00:41:23Z

@bobbyangers maybe you could start on supporting multi-tenancy while I do the remaining refactoring?
if you needed to expose anything from the public api I will help with it

bobbyangers · 2021-11-07T23:33:19Z

@bobbyangers maybe you could start on supporting multi-tenancy while I do the remaining refactoring? if you needed to expose anything from the public api I will help with it

Hi @ahmednfwela, yes I will do that, but as an contrib extension, not in the main package.

dj-nitehawk · 2021-11-10T05:12:31Z

MongoDB.Entities/Core/FileEntity.cs

-        /// <summary>
-        /// Access the DataStreamer class for uploading and downloading data
-        /// </summary>
-        public DataStreamer Data => streamer ??= new DataStreamer(this);


do we need to break the api here? or will you be adding this back in later?

yes, since DataStreamer needs to get FileChunk collection, it needs to know about the DBContext, so we would either have to

pass the DBContext to the FileEntity just to be used in new DataStreamer(this, db);

or leave the creation of DataStreamer to the DBContext (better)

ahmednfwela · 2021-11-10T11:25:26Z

@dj-nitehawk @bobbyangers I made the DBContext api fully customizable, each method can be passed an optional string? collectionName or IMongoCollection<T>? collection
which can override the default behavior of the collection resolution.

this means that we don't even assume collections are in the same database as the context, e.g.

Context.Find<T>(collection: someCollectionInAnotherDataBase)

this is useful for applying OnBeforeSave and OnBeforeUpdate on arbitrary collections, and for multi tenancy

bobbyangers · 2021-11-19T15:33:44Z

@ahmednfwela can you reinvite me please ? I seems, that my initial confirmation was incomplete.

Also; I also wanted to check with you; is the project compiling now ?

ahmednfwela · 2021-11-19T16:46:37Z

@bobbyangers well, not yet, I am a little busy nowadays, so I didn't have time to finish yet.

bobbyangers · 2021-11-19T17:41:56Z

My issue at the moment is that I cannot compile, nor verify if my changes are ok. When you have time let me know.

…

--Robert Angers m:514-557-8074

Sent from my iPhone On Nov 19, 2021, at 11:46, Ahmed Fwela ***@***.***> wrote: @bobbyangers<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fbobbyangers&data=04%7C01%7C%7C2aec0941a34f4a8a10ba08d9ab7c2da4%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637729372106112565%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=W9%2F7OklW48i6ASDrjj0c9XAvwhbSXMR%2FwrQqyABwqCY%3D&reserved=0> well, not yet, I am a little busy nowadays, so I didn't have time to finish yet. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdj-nitehawk%2FMongoDB.Entities%2Fpull%2F125%23issuecomment-974233560&data=04%7C01%7C%7C2aec0941a34f4a8a10ba08d9ab7c2da4%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637729372106112565%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GiaMZO8MJPhpoTxMROx84NmNG0kXk3MXMoB9n378Vn0%3D&reserved=0>, or unsubscribe<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAANL52ZWUFCP3QYPGXOSVALUMZ5PPANCNFSM5HM66YXQ&data=04%7C01%7C%7C2aec0941a34f4a8a10ba08d9ab7c2da4%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637729372106122519%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=MAQewAM93xduLXsExOSlaERGsLzzl04sKRgbIdwZOxw%3D&reserved=0>.

dj-nitehawk · 2021-12-08T14:59:24Z

@ahmednfwela you should pull the latest changes from dj-nitehawk:multi-tenancy branch. did a small hotfix for #146

bobbyangers · 2021-12-28T03:26:54Z

@ahmednfwela what is the status of the work ? And what is left to do ?

ahmednfwela · 2021-12-28T03:31:37Z

I am currently researching how to do relationships efficiently.
I am also migrating the static api to use singleton pattern instead, but to be honest I would rather just remove it altogether, since it's not worth the effort

dj-nitehawk · 2021-12-28T03:36:17Z

@ahmednfwela ok let's remove the static stuff if it makes it easier to maintain the project in the long-run. so yeah go ahead, rip that shit out 😜

ahmednfwela · 2021-12-28T03:57:30Z

well, that took a load off my chest

dj-nitehawk · 2021-12-28T04:05:24Z

@ahmednfwela have you given any thought to how migrations should be done now?
where are we gonna store the migration history when it comes to multi-tenant?
and are we gonna execute each new migration against all tenant databases, etc. etc.

ahmednfwela · 2021-12-28T04:15:17Z

well, I am going to move migration to be per DBContext as well, this way we don't care how many databases the user has

dj-nitehawk · 2021-12-28T04:17:39Z

@ahmednfwela i added you as a collaborator to this repo. here's what i'm thinking about how to proceed with this refactor.

let's bring in your current code to a new branch on this repo called "vNext"
work/commit directly on that branch.
once we have tests passing, let's release an alpha version so we can test it in the wild.
then create a new "vNext-Docs" branch and get the docs ready for the v22
once the new codebase is stable enough (after a couple of betas), we'll move the current master branch to a "v21-Legacy" branch and make vNext the master.

what do you think?

ahmednfwela · 2021-12-28T04:19:47Z

sure, that sounds like a good plan!

ahmednfwela · 2021-12-28T04:20:40Z

vNext should be branched off from multi-tenancy though

dj-nitehawk · 2021-12-28T04:21:24Z

vNext should be branched off from multi-tenancy though

yeah go ahead and create it however you like. or just rename "multi-tenancy" to "vNext" and pull in your changes.

ahmednfwela · 2021-12-28T04:27:04Z

done

dj-nitehawk · 2021-12-28T04:32:20Z

@ahmednfwela will you merge and close this PR into vnext? or you wanna keep working on your fork and merge later?

ahmednfwela · 2021-12-28T04:33:33Z

I will merge it to vNext once it can actually compile, then we can do multiple PRs onto vNext later

ahmednfwela · 2021-12-28T04:39:25Z

I also removed all dependency on IEntity/IEntity<T> and move their functionality to extensions
this means that queries can run on ANY type, instead of only types implementing IEntity

ahmednfwela · 2022-04-01T04:27:51Z

@dj-nitehawk i will merge this for now (even tho project isn't building)

ahmednfwela added 5 commits November 5, 2021 06:06

.

9a8988b

Initial WIP

218bb9e

added MongoContextOptions

ff8b6fb

WIP moving static implmentations to MongoContext and DBContext

2db41d0

revert back TargetFramework

e4d479a

dj-nitehawk mentioned this pull request Nov 5, 2021

Q: Support for multi-tenant ? #123

Open

Better find

c4d4376

ahmednfwela added 3 commits November 5, 2021 16:40

WIP missing Update, UpdateAndGet from builders

62362a1

WIP update

0ccb4e6

WIP

da7e4d9

* Major refactoring * No more repeating code in builders * Multi-tenancy is per database now instead of per collection, which should be cleaner

ahmednfwela added 2 commits November 6, 2021 02:10

WIP

2e15d59

Rename

389d86f

ahmednfwela added 3 commits November 9, 2021 18:39

fixed Delete

2d93a7d

reworked builders

936633a

WIP insert

9595d38

dj-nitehawk reviewed Nov 10, 2021

View reviewed changes

DBContext is done

0141761

WIP JoinRecord

7f6a312

dj-nitehawk mentioned this pull request Nov 14, 2021

I don't understand to declare connection #120

Closed

WIP attempt to remove dependency on IEntity

84d9bbf

remove static API

4467815

WIP

e1aba55

merge changes

c3c370a

WIP extensions

899df7a

dj-nitehawk mentioned this pull request Dec 28, 2021

WebApp not starting - when connection string is bad #149

Closed

merge conflict

a61f407

ahmednfwela marked this pull request as ready for review April 1, 2022 04:28

ahmednfwela merged commit c4eed0b into dj-nitehawk:vnext Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api restructure #125

Api restructure #125

ahmednfwela commented Nov 5, 2021 •

edited

Loading

dj-nitehawk commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021

dj-nitehawk commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021

dj-nitehawk commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021 •

edited

Loading

dj-nitehawk commented Nov 5, 2021

bobbyangers commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021

bobbyangers commented Nov 6, 2021

ahmednfwela commented Nov 6, 2021

bobbyangers commented Nov 7, 2021

dj-nitehawk Nov 10, 2021

ahmednfwela Nov 10, 2021

ahmednfwela commented Nov 10, 2021

bobbyangers commented Nov 19, 2021

ahmednfwela commented Nov 19, 2021

bobbyangers commented Nov 19, 2021 via email

dj-nitehawk commented Dec 8, 2021

bobbyangers commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021 •

edited

Loading

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021 •

edited

Loading

ahmednfwela commented Apr 1, 2022

Api restructure #125

Api restructure #125

Conversation

ahmednfwela commented Nov 5, 2021 • edited Loading

dj-nitehawk commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021

dj-nitehawk commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021

dj-nitehawk commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021 • edited Loading

dj-nitehawk commented Nov 5, 2021

bobbyangers commented Nov 5, 2021

ahmednfwela commented Nov 5, 2021

bobbyangers commented Nov 6, 2021

ahmednfwela commented Nov 6, 2021

bobbyangers commented Nov 7, 2021

dj-nitehawk Nov 10, 2021

Choose a reason for hiding this comment

ahmednfwela Nov 10, 2021

Choose a reason for hiding this comment

ahmednfwela commented Nov 10, 2021

bobbyangers commented Nov 19, 2021

ahmednfwela commented Nov 19, 2021

bobbyangers commented Nov 19, 2021 via email

dj-nitehawk commented Dec 8, 2021

bobbyangers commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021 • edited Loading

ahmednfwela commented Dec 28, 2021

dj-nitehawk commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021

ahmednfwela commented Dec 28, 2021 • edited Loading

ahmednfwela commented Apr 1, 2022

ahmednfwela commented Nov 5, 2021 •

edited

Loading

ahmednfwela commented Nov 5, 2021 •

edited

Loading

dj-nitehawk commented Dec 28, 2021 •

edited

Loading

ahmednfwela commented Dec 28, 2021 •

edited

Loading