ethanresnick / json-api

Turn your node app into a JSON API server (http://jsonapi.org/)
GNU Lesser General Public License v3.0
268 stars 41 forks source link

Legit parsing #162

Closed ethanresnick closed 6 years ago

ethanresnick commented 6 years ago

This implements the filter format changes described in #160. Along the way, it replaces the ad-hoc lexer/parser I had before with a proper parser generated from a formal PEG grammar. That grammar is here.

Because the new filter syntax is more generic, the parse result for a standard field constraint is now

{ operator: string, args: any[] }

which is more flexible than the old

{ operator: string, field: string; value: any }

Even with this extra flexibility (e.g., for use by future operators), though, adapters don't have to worry about the data coming in in an unexpected format because, for the built-in binary operators, the library validates that args[0] is a field reference and args[1] is what would've been in .value before. (See UPGRADING.md). Still, this requires a more-involved migration than I'd like ideally, so I'm open to adding a smoother transition path. On the other hand, v3 is so close, after which this'll all be fixed, that it may not be necessary.

@carlbennettnz If you have any thoughts, lmk.

codecov-io commented 6 years ago

Codecov Report

Merging #162 into v3-evolution-over-rewrite will increase coverage by 0.26%. The diff coverage is 92.56%.

Impacted file tree graph

@@                      Coverage Diff                      @@
##           v3-evolution-over-rewrite     #162      +/-   ##
=============================================================
+ Coverage                      90.67%   90.93%   +0.26%     
=============================================================
  Files                             56       56              
  Lines                           2199     2152      -47     
  Branches                         502      477      -25     
=============================================================
- Hits                            1994     1957      -37     
+ Misses                           205      195      -10
Impacted Files Coverage Δ
src/types/Query/DeleteQuery.ts 100% <ø> (ø) :arrow_up:
src/types/Query/FindQuery.ts 50% <ø> (ø) :arrow_up:
src/db-adapters/Mongoose/MongooseAdapter.ts 96.27% <100%> (ø) :arrow_up:
src/db-adapters/Mongoose/lib.ts 95.12% <100%> (-0.54%) :arrow_down:
src/controllers/API.ts 94.05% <100%> (+0.72%) :arrow_up:
src/index.ts 97.29% <100%> (+0.15%) :arrow_up:
src/types/Query/WithCriteriaQuery.ts 78% <65%> (-6.45%) :arrow_down:
src/util/query-parsing.ts 93.33% <93.33%> (ø)
src/steps/pre-query/parse-query-params.ts 91.42% <96.96%> (+4.94%) :arrow_up:

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 572945d...d33d57c. Read the comment docs.

carlbennettnz commented 6 years ago

Looking good. Thanks for your work on this. Should be a big improvement.

the library validates that args[0] is a field reference and args[1] is what would've been in .value before

Did you manage to have this reflected by the TypeScript types? It that even possible?

ethanresnick commented 6 years ago

Looking good. Thanks for your work on this. Should be a big improvement.

Awesome; I'm gonna merge this, then, and publish beta 25 soon. After that, I think we're pretty close to RC stage. The big thing left is updating the adapter method return types.

Did you manage to have this reflected by the TypeScript types? It that even possible?

Yeah, it's reflected here, with the tuple type and Identifier being the first entry.

A real limitation with the typings, though, is that, in theory, a custom adapter can provide its own validation functions and supported operators set to allow a totally different structure, and then the types won't reflect that. I'm not sure yet how to set up the types so that they can be extended in the same way that the actual parsing can be extended. For now, though, I don't think it's worth worrying about.