Introduces flow types for storage #4349

flovilmart · 2017-11-14T18:06:02Z

No description provided.

steven-supersolid · 2017-11-14T18:36:29Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

@@ -78,20 +87,20 @@ const mongoSchemaFromFieldsAndClassNameAndCLP = (fields, className, classLevelPe
 }


-export class MongoStorageAdapter {
+export class MongoStorageAdapter implements StorageAdapter, IndexingStorageAdapter {
  // Private
  _uri: string;
  _collectionPrefix: string;
  _mongoOptions: Object;
  // Public
  connectionPromise;


Is it possible to add a type here or not worthwhile?

probably yes,

codecov · 2017-11-14T18:50:43Z

Codecov Report

Merging #4349 into master will decrease coverage by 0.04%.
The diff coverage is 94.85%.

@@            Coverage Diff             @@
##           master    #4349      +/-   ##
==========================================
- Coverage   92.84%   92.79%   -0.05%     
==========================================
  Files         118      118              
  Lines        8381     8372       -9     
==========================================
- Hits         7781     7769      -12     
- Misses        600      603       +3

Impacted Files	Coverage Δ
src/Adapters/Files/GridStoreAdapter.js	`100% <ø> (ø)`	⬆️
src/Push/PushWorker.js	`93.1% <ø> (ø)`	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`95.78% <100%> (+0.33%)`	⬆️
src/Controllers/HooksController.js	`96.15% <100%> (ø)`	⬆️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js	`95.18% <80%> (-0.98%)`	⬇️
src/Controllers/SchemaController.js	`96.47% <86.36%> (-0.65%)`	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`96.87% <90%> (-0.2%)`	⬇️
src/Controllers/DatabaseController.js	`94.56% <95.61%> (-0.26%)`	⬇️
src/Adapters/Auth/meetup.js	`84.21% <0%> (-5.27%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0f1870...ef882d3. Read the comment docs.

montymxb · 2017-11-14T19:00:12Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1489,7 +1495,7 @@ export class PostgresStorageAdapter {
      }
    }

-    const qs = `SELECT ${columns} FROM $1:name ${wherePattern} ${sortPattern} ${limitPattern} ${skipPattern} ${groupPattern}`;
+    const qs = `SELECT ${columns.join(',')} FROM $1:name ${wherePattern} ${sortPattern} ${limitPattern} ${skipPattern} ${groupPattern}`;


Wait, how did the tests not catch this? Seems like this would have caused a noticeable problem.

yeah that's very very odd. @dplewis ?

Good catch. There isn't a test for project with multiple fields, only a test with 1 field. This should fix that issue.

montymxb · 2017-11-14T19:07:46Z

src/Controllers/DatabaseController.js

@@ -117,75 +122,6 @@ const validateQuery = query => {
  });
 }

-function DatabaseController(adapter, schemaCache) {


@flovilmart could you fill me on the rationale for the changes in this file? There's a lot that's been shifted around in the diff and I'm not entirely sure what the objective is here.

Make it a class, and also make it conform to a particular interface, so it’s easier to know what’s consumed and how in the future (like the indexing) this also rationalizes to a certain extent also the interfaces for every calls to the adapters.

Ah, now I see! Thanks for clarifying.

flovilmart · 2017-11-14T23:00:12Z

@montymxb I could go further, formalize Schema and SchemaController as it's quite tough to follow there what's what. What do you think?

montymxb · 2017-11-15T06:15:14Z

@flovilmart I think we could go further, but for now I think this is good for the PR 👍 . Any other improvements to structure are always good though.

Just to note I recently took some time to go back through the source again for parse-server. Last time I really went through it all was when this was all first open sourced with fosco. A ton has changed since then, figured I needed to go back over it all.

Looking at it all it's been really cool to see everything that's been built into this, but it's been a lot of growth. I think it could use some trimming/restructuring now to standardize and clean things up. In the process of doing that it would be nice to roadmap the layout that is parse server, in a general sense kind of like this for the php sdk but not necessarily in graphical form. Just something that we can add to the docs that can help someone grasp things like:

What Parse Server is
How to use it
How it works (generally)
How it works (under the hood)

That's a lot of work to put together docs for stuff like that, but if we organize it so we do it with a general code cleanup/restructures we could do it piece by piece. Whenever we have time. Once the cleanup is done the documented portions of that code would be done as well (hopefully).

Just some thoughts, we could open up a project for this to itemize what would need to be done. May not even need to actually do a code restructure either, maybe just cleanup some TODOs and such. Either way, it would be an excuse to make the project a bit more transparent and easy to understand.

dplewis · 2017-11-15T15:44:36Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1627,7 +1633,7 @@ function literalizeRegexPart(s) {

  // process regex that has a beginning specified for the literal text
  const matcher2 = /\\Q((?!\\E).*)$/
-  const result2 = s.match(matcher2);
+  const result2: any = s.match(matcher2);
  if(result2 && result2.length > 1 && result2.index > -1){


Shouldn't the linter catch spaces in if statements. if () {

montymxb · 2017-12-08T00:36:40Z

@flovilmart are we still pushing for this?

flovilmart · 2017-12-08T00:37:30Z

Needs some rework

acinader · 2017-12-08T00:40:37Z

spec/MongoStorageAdapter.spec.js

@@ -1,6 +1,6 @@
 'use strict';

-const MongoStorageAdapter = require('../src/Adapters/Storage/Mongo/MongoStorageAdapter');
+const MongoStorageAdapter = require('../src/Adapters/Storage/Mongo/MongoStorageAdapter').MongoStorageAdapter;


the newer airbnb wants:

const {MongoStorageAdapter} = require('../src/Adapters/Storage/Mongo/MongoStorageAdapter');

all the cool kids are doing it.

lol :) most definitely :)

dplewis · 2017-12-29T22:42:45Z

@flovilmart Do you know why find succeeds when query is within maxTimeMS keeps failing?

montymxb · 2017-12-29T23:10:42Z

Wanted to note that I've been keeping a small log of the arbitrary errors we see in CI. That one in particular is the most common from what I've observed.

- runs flow on pre tests - fixes flow

Fixing invalid database code in method `setIndexesWithSchemaFormat`: * It must be a transaction, not a task, as it executes multiple database changes * It should contain the initial queries inside the transaction, providing the context, not outside it; * Replaced the code with the ES6 Generator notation * Removing the use of batch, as the value of the result promise is irrelevant, only success/failure that matters

flovilmart · 2017-12-30T19:23:18Z

@montymxb @dplewis can we get a look at this one? With the refactoring in PG, it becomes costly to maintain this :)

dplewis · 2017-12-30T20:11:04Z

LGTM! Good work flo on getting flow types working. Let’s get this in before it gets stale.

* Introduces flow types for storage * Better typing of QueryOptions * Adds flow types to SchemaCOntroller, - runs flow on pre tests - fixes flow * Adds ClassLevelPermissions type * Moves Controller types into a single file * Changes import styles * Changes import styles * fixing method setIndexesWithSchemaFormat (parse-community#4454) Fixing invalid database code in method `setIndexesWithSchemaFormat`: * It must be a transaction, not a task, as it executes multiple database changes * It should contain the initial queries inside the transaction, providing the context, not outside it; * Replaced the code with the ES6 Generator notation * Removing the use of batch, as the value of the result promise is irrelevant, only success/failure that matters * nits * Fixes tests, improves flow typing

flovilmart requested review from dplewis, montymxb and acinader November 14, 2017 18:06

steven-supersolid reviewed Nov 14, 2017

View reviewed changes

montymxb reviewed Nov 14, 2017

View reviewed changes

flovilmart force-pushed the flow-type-storage-adapter branch from 8d8f4c6 to 34a4d27 Compare November 14, 2017 20:58

dplewis reviewed Nov 15, 2017

View reviewed changes

flovilmart force-pushed the flow-type-storage-adapter branch from 2d8ce40 to 37fd074 Compare November 25, 2017 20:09

flovilmart added this to the 2.7.0 milestone Nov 25, 2017

flovilmart force-pushed the flow-type-storage-adapter branch 2 times, most recently from 3f333b3 to 0c8180f Compare November 25, 2017 21:36

acinader previously approved these changes Dec 8, 2017

View reviewed changes

flovilmart dismissed acinader’s stale review via 4450207 December 29, 2017 18:49

flovilmart force-pushed the flow-type-storage-adapter branch 2 times, most recently from 01f9da7 to 29bfacd Compare December 29, 2017 20:28

flovilmart force-pushed the flow-type-storage-adapter branch from 508fce7 to d7b226e Compare December 30, 2017 18:58

flovilmart added 7 commits December 30, 2017 14:21

Introduces flow types for storage

5474895

Better typing of QueryOptions

73e22a1

Adds flow types to SchemaCOntroller,

016f4d3

- runs flow on pre tests - fixes flow

Adds ClassLevelPermissions type

979e4d7

Moves Controller types into a single file

439825b

Changes import styles

148cc07

Changes import styles

c357bd3

vitaly-t and others added 3 commits December 30, 2017 14:22

nits

277bdec

Fixes tests, improves flow typing

ef882d3

flovilmart force-pushed the flow-type-storage-adapter branch from d7b226e to ef882d3 Compare December 30, 2017 19:22

flovilmart merged commit 1063137 into master Dec 31, 2017

flovilmart deleted the flow-type-storage-adapter branch December 31, 2017 01:44

montymxb mentioned this pull request Dec 31, 2017

#4338 pg schema upgrade #4375

Merged

dplewis mentioned this pull request Apr 21, 2022

refactor: upgrade @graphql-tools/links from 8.2.7 to 8.2.8 #7945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces flow types for storage #4349

Introduces flow types for storage #4349

flovilmart commented Nov 14, 2017

steven-supersolid Nov 14, 2017

flovilmart Nov 14, 2017

codecov bot commented Nov 14, 2017 •

edited

Loading

montymxb Nov 14, 2017

flovilmart Nov 14, 2017

dplewis Nov 14, 2017 •

edited

Loading

montymxb Nov 14, 2017

flovilmart Nov 14, 2017

montymxb Nov 14, 2017

flovilmart commented Nov 14, 2017

montymxb commented Nov 15, 2017 •

edited

Loading

dplewis Nov 15, 2017

montymxb commented Dec 8, 2017

flovilmart commented Dec 8, 2017

acinader Dec 8, 2017

flovilmart Dec 8, 2017

dplewis commented Dec 29, 2017

montymxb commented Dec 29, 2017

flovilmart commented Dec 30, 2017

dplewis commented Dec 30, 2017 •

edited

Loading

Introduces flow types for storage #4349

Introduces flow types for storage #4349

Conversation

flovilmart commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplewis Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flovilmart commented Nov 14, 2017

montymxb commented Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

montymxb commented Dec 8, 2017

flovilmart commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplewis commented Dec 29, 2017

montymxb commented Dec 29, 2017

flovilmart commented Dec 30, 2017

dplewis commented Dec 30, 2017 • edited Loading

codecov bot commented Nov 14, 2017 •

edited

Loading

dplewis Nov 14, 2017 •

edited

Loading

montymxb commented Nov 15, 2017 •

edited

Loading

dplewis commented Dec 30, 2017 •

edited

Loading