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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike: booter for creating REST APIs from model files #3617

Closed
wants to merge 13 commits into from

Conversation

@bajtos
Copy link
Member

commented Aug 29, 2019

Quoting from #2036:

In LoopBack 3, it was very easy to get a fully-featured CRUD REST API with
very little code: a model definition describing model properties + a model
configuration specifying which datasource to use.

Let's provide the same simplicity to LB4 users too.

  • User creates a model class and uses decorators to define model properties.
    (No change here.)
  • User declaratively defines what kind of data-access patterns to provide
    (CRUD, KeyValue, etc.) and what datasource to use under the hood.
  • @loopback/boot processes this configuration and registers appropriate
    repositories & controllers with the app.

In this Spike, I am demonstrating a PoC implementation of an extensible booter
that processed model configuration files in JSON formats and uses 3rd-party
plugins to build repository & controller classes at runtime.

See SPIKE.md for further details.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@bajtos bajtos requested a review from raymondfeng Aug 29, 2019

@bajtos bajtos self-assigned this Aug 29, 2019

packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/inject.ts Outdated Show resolved Hide resolved
packages/context/src/binding-inspector.ts Outdated Show resolved Hide resolved
// Start Application
await app.start();
}

async function givenTodoRepository() {
todoRepo = await app.getRepository(TodoRepository);
todoRepo = await app.get<TodoRepository>('repositories.TodoRepository');

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 29, 2019

Author Member

We don't have a repository class we could use here, TodoRepository is just a type alias for EntityCrudRepository<Todo, number>.

import {GeocoderService} from '../../../services';
import {aLocation, givenTodo} from '../../helpers';

describe('TodoController', () => {

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 29, 2019

Author Member

I see little value in writing unit tests for controllers contributed by 3rd-party packages. The controller code should have been already tested in that package (@loopback/rest-crud in this case).

import {TodoRepository} from '../repositories';
import {GeocoderService} from '../services';

export class TodoController {

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 29, 2019

Author Member

No controller files, yay 馃帀

import {DefaultCrudRepository, juggler} from '@loopback/repository';
import {Todo, TodoRelations} from '../models';

export class TodoRepository extends DefaultCrudRepository<

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 29, 2019

Author Member

No repository files, yay 馃帀

type: 'string',
})
remindAtGeo?: string; // latitude,longitude

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 29, 2019

Author Member

In order to implement geocoder integration while using public-models/product.config.json, we need to customize the controller method defined by @loopback/rest-crud. That's kind of against the spirit of this change, where we want to use the default REST API as provided by LB. At least for the initial release, I'd like us to recommend users to switch from @loopback/rest-crud to lb4 repository & lb4 controller if they need any tweaks in the built-in functionality.

@bajtos bajtos referenced this pull request Aug 29, 2019
0 of 3 tasks complete

@bajtos bajtos force-pushed the spike/crud-rest-booter branch 2 times, most recently from 12103a0 to e116ab5 Aug 30, 2019

@bajtos bajtos requested review from strongloop/sq-lb-apex Aug 30, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

The spike is done and ready for review.

packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/inject.ts Outdated Show resolved Hide resolved
@raymondfeng raymondfeng referenced this pull request Aug 30, 2019
3 of 7 tasks complete
_SPIKE_.md Outdated Show resolved Hide resolved
@jannyHou
Copy link
Contributor

left a comment

馃憤 馃憤 Great effort. I left a few comments regarding the flexibility to modify the apis defined in controller class.

The todo example looks simple enough to config 馃憤

_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
@bajtos bajtos referenced this pull request Sep 6, 2019
3 of 3 tasks complete

@bajtos bajtos force-pushed the spike/crud-rest-booter branch 2 times, most recently from b7c6feb to 67d1128 Sep 6, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

I have updated the spike as follows:

  • Rebased on top of the latest master
  • Reworked the implementation to load model configs from JS files in dist/public-models/*.config.js instead of public-models/*.config.json
  • Moved RestBooter to @loopback/boot and renamed it to ModelApiBooter
  • Improved SPIKE docs based on review comments, updated list of implementation stories

I consider this spike as done and ready for final review & approval.

@strongloop/loopback-next PTAL.

@bajtos bajtos requested review from strongloop/loopback-next, raymondfeng and jannyHou Sep 6, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@bajtos Great work! 馃 I love the general direction to allow pluggable builders to create/derive/compose new artifacts from existing ones.

See my comments inline.

@bajtos bajtos referenced this pull request Sep 9, 2019
0 of 9 tasks complete
@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

@raymondfeng thank you for a great feedback! Let's keep the discussion going in the inline comments.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Over the weekend, I realized that if we want to keep ModelApiBuilder generic and support more API flavors besides REST, then basePath configuration does not belong the the generic config interface. In 4f74b98, I introduced a new config interface CrudRestApiConfig (inheriting from ModelApiConfig) and moved basePath there.

I am not sure if CrudRestApiConfig is the best name (it does not mention "model"), suggestions are welcome.

@agnes512
Copy link
Contributor

left a comment

Nice write up 馃憤 It's easy to get idea of the spike along with the model-api.booter test`.

_SPIKE_.md Outdated

## Basic use

Create `src/public-models` directory in your project. For each model you want to

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 9, 2019

Contributor

I prefer model-endpoints as the directory name for the models.
As for config files, I think names with config makes more sense to me. I am ok with model-api-config or api-config.

bajtos added 9 commits Aug 26, 2019
feat: version 1
- feat(booter-rest): initial commit
- feat: first draft of the implementation
- feat: model-api builder and rest-crud implementation
- feat(example-todo): rework the code to use rest-crud component
- docs: add more spike docs
refactor: load model configs from JS files in `dist/public-models`
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
refactor: move RestBooter to `@loopback/boot` as ModelApiBooter
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
docs: more explanation for extensibility
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
fix: copyright headers
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
docs: finalize the spike
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
fixup! address review comments (refactor-rename)
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
refactor: move basePath to rest-crud config iface
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
chore: cleanup
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>

@bajtos bajtos force-pushed the spike/crud-rest-booter branch from 4f74b98 to 446d5f5 Sep 10, 2019

refactor: import models directly, change file/dir naming scheme
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Thank you all for feedback. I decided to address it the following way:

  • I have renamed public-models to model-endpoints as suggested in the comments.
  • I am using the naming convention {model-name}.rest-config.ts, this will allow us to introduce e.g. {model-name}.grpc-config.ts in the future.
  • The ModelApiConfig object is referencing the model class directly now, setting model field to an imported class constructor.
  • I have removed app.model() API that's no longer needed.
  • I have update the SPIKE document accordingly.

Let's do a final round of reviews and then call the spike done.

refactor: revert unnecessary changes
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@hacksparrow

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

I am not sure if CrudRestApiConfig is the best name (it does not mention "model"), suggestions are welcome.

Since we are focused on REST for now CrudRestApiConfig sound OK to me. And it resides under @loopback/crud-rest.

If we support more flavors, they'll have their own names and reside in their own package, if not in a generic one where CrudRestApiConfig may also reside.

@hacksparrow

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Oh, just realized your other point. I think it is important to give the context to CrudRestApiConfig like mode-endpoints. So maybe ModelCrudRestApiConfig?

Simply CrudRestApiConfig sounds like the LoopBack default, which it's not.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

Oh, just realized your other point. I think it is important to give the context to CrudRestApiConfig like mode-endpoints. So maybe ModelCrudRestApiConfig?

Simply CrudRestApiConfig sounds like the LoopBack default, which it's not.

Exactly!

I'll update the interface to ModelCrudRestApiConfig then. (It's an awfully long name though 馃檲 )

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

It's ok if we don't get the names exactly right in the spike, we can tweak them later during code review of the real implementation.

feat: rename CrudRestApiConfig to ModelCrudRestApiConfig
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@bajtos bajtos referenced this pull request Sep 13, 2019
0 of 3 tasks complete
docs: add links to follow-up tasks
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@bajtos

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Follow-up stories:

  • Implement TestSandbox.writeTextFile #3731
  • Improve defineCrudRestController to create a named controller class #3732
  • Add defineCrudRepositoryClass - a helper to create a named repository class #3733
  • Model API booter & builder #3736
  • Add CrudRestApiBuilder to @loopback/rest-crud #3737
  • Example app showing CrudRestApiBuilder #3738

I am closing the spike as done.

@bajtos bajtos closed this Sep 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.