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

DBController refactoring #1228

Merged
merged 3 commits into from
Apr 14, 2016
Merged

Conversation

flovilmart
Copy link
Contributor

Removes adaptiveCollection from DatabaseController

  • All collections manipulations are now handled by a DBController
  • Adds .Unsafe on DBController that generates a DBController that skips className validations and key names validations (this allows using the DBController for internal classes like _PushStatus and 'private' properties like _perishable_token)
  • Adds ability to configure RestWrite with multiple writes
  • Moves some transfirmations to MongoTransform as they output specific code

@codecov-io
Copy link

Current coverage is 92.99%

Merging #1228 into master will increase coverage by +0.13% as of 60a8c57

@@            master   #1228   diff @@
======================================
  Files           87      87       
  Stmts         5491    5497     +6
  Branches      1015    1019     +4
  Methods          0       0       
======================================
+ Hit           5099    5112    +13
+ Partial         10       9     -1
+ Missed         382     376     -6

Review entire Coverage Diff as of 60a8c57

Powered by Codecov. Updated on successful CI builds.

@drew-gross
Copy link
Contributor

Adds optional flags to configure an unsafe databaseController for direct
access

I don't think we want this. Obviously we wouldn't be able to use it internally, or the entire concept of database adapters won't work. If someone wants direct DB access in their cloud code, they can just use the driver like they would if they weren't writing a Parse app.

@flovilmart
Copy link
Contributor Author

This is not to provide direct DB access, but skip className and key validation so we can use The DB controller instead of the adaptive collection. Problem with adaptive collection is that it's highly opinionated towards the driver.

Updated the misleading description

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from f29495d to 55d91e4 Compare March 29, 2016 02:00
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@drew-gross
Copy link
Contributor

Rather than allowing the "unsafe" version to do anything at all, can we just allow it to write classes and fields that start with an underscore? Also I think @nlutsenko's strategy with the adaptive collection was to first make it less tightly coupled, and then make it less opinionated in a separate phase.

@flovilmart
Copy link
Contributor Author

That's the goal of the unsafe. Not sure we're not saying the same thing differently.

The problem with the adaptiveCollection abstraction, is that it would force to move all transformations inside the collection instead of outside, in DBController

---> REST format ---> DBController -- adapter.transform -> adaptiveCollection

With this architecture the adapter is fully and solely responsible to transform, untransform, store and retrieve data. The DBController orchestrates all operations, and takes REST format and returns REST format. The rest of the module only talks in REST to the DBAdapater, abstracting to the right level.

For a SQL DB, as the transform would spit out strings that would be passed as is to the adapativeCollection. AdaptiveCollectoon means that it's an adapter specific collection, not a collection that takes a standard query format.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from 55d91e4 to fed1949 Compare March 29, 2016 18:52
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from fed1949 to 7c2020d Compare March 29, 2016 20:52
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@nlutsenko
Copy link
Contributor

Sorry, sir, looks like this needs rebase. :) Pass back hen you are done.

@flovilmart
Copy link
Contributor Author

@nlutsenko it's all yours!

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from 7426d18 to b98ca44 Compare April 7, 2016 14:26
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from b98ca44 to 56c97bb Compare April 8, 2016 18:16
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from 56c97bb to cd7e09e Compare April 11, 2016 23:25
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart
Copy link
Contributor Author

@drew-gross , @nlutsenko what should we do with that PR? That makes the internals use the proper Rest format, I'm open to refactor differently/further if needed.

@drew-gross
Copy link
Contributor

I had been planning to let @nlutsenko continue with reviewing since he has done more of the db adapter code, and has more context on how it should work.

@nlutsenko
Copy link
Contributor

I'll take a look at it after F8 (which is tomorrow and Wednesday).
Sorry it takes that much time to get to it, am tangled in other things...

@flovilmart
Copy link
Contributor Author

Good luck for F8! Let me know when you get some time on slack, if it needs a rebase at that time I'll do a quick one.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from 1b5d53b to a568a98 Compare April 12, 2016 23:10
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from a568a98 to 2cdfb25 Compare April 14, 2016 01:40
- Adds ACL query injection in MongoTransform
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from 2cdfb25 to 035146b Compare April 14, 2016 01:47
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

- All collections manipulations are now handled by a DBController
- Adds optional flags to configure an unsafe databaseController for direct
  access
- Adds ability to configure RestWrite with multiple writes
- Moves some transfirmations to MongoTransform as they output specific code
@flovilmart flovilmart force-pushed the flovilmart.refactorMongoTransform branch from 035146b to 5c7c7e4 Compare April 14, 2016 13:40
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@drew-gross
Copy link
Contributor

I think @nlutsenko is busy and this looks pretty solid to me and passes tests. The DB Adapter API will need a lot of changes anyway so I say might as well merge.

@flovilmart
Copy link
Contributor Author

Alright then!

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.

5 participants