herbsjs / herbs-cli

Herbs CLI
https://herbsjs.org/
MIT License
30 stars 30 forks source link

Find Item - Naming problems and multiples IDs allowed #61

Open dalssoft opened 3 years ago

dalssoft commented 3 years ago

apart from the naming, this issue rises a problem with being able to get multiples IDs from REST.

This issue highlits

  1. Fix file name
  2. Fix UC and step description
  3. Allow the same UC to handle single and multiple IDs request. /item/ (all), /item/?ids=1,2,3 (multiples) and /item/2 (single)

It is still needed:

  1. A fix to REST layer to handle /item/ (all), /item/?ids=1,2,3 (multiples) and /item/2 (single)

current:

getByIdItem.js:

const { usecase, step, Ok, Err } = require('@herbsjs/herbs')
const { Item } = require('../../entities')

const useCase = ({ itemRepository }) => () =>
  usecase('Find one Item', {
    // Input/Request metadata and validation 
    request: {
      id: Number,
    },

    // Output/Response metadata
    response: Item,

    //Authorization with Audit
    // authorize: user => (user.canFindOneItem ? Ok() : Err()),
    authorize: user => Ok(user),

    'Find the Item': step(async ctx => {
      // ctx.ret is the Use Case return
      const [result] = await itemRepository.findByID(parseInt(ctx.req.id)) 
      if (!result) return Err.notFound({ 
        message: `Item entity not found by id: ${ctx.req.id}`,
        payload: { entity: 'Item' }
      })

      return (ctx.ret = Item.fromJSON(result))
    })
  })

module.exports = useCase

should be:

findItemByIDs.js:

const { usecase, step, Ok, Err } = require('@herbsjs/herbs')
const { Item } = require('../../entities')

const useCase = ({ itemRepository }) => () =>
  usecase('Find Item by IDs', {
    // Input/Request metadata and validation 
    request: {
      ids: [Number],
    },

    // Output/Response metadata
    response: [Item],

    //Authorization with Audit
    // authorize: user => (user.canFindOneItem ? Ok() : Err()),
    authorize: user => Ok(user),

    'Find Item(s)': step(async ctx => {
      // ctx.ret is the Use Case return
      const result = await itemRepository.findByID(ctx.req.ids)
      if (!result) return Err.notFound({ 
        message: `Item entity not found by IDs: ${ctx.req.ids}`,
        payload: { entity: 'Item' }
      })

      return (ctx.ret = Item.fromJSON(result))
    })
  })

module.exports = useCase
italojs commented 3 years ago

I think express understand the /item?ids=1,2,3 different then /item/123 What do you think about we merge this feature with https://github.com/herbsjs/herbs-cli/issues/21 issue? My idea is create the getAll route with a query filter, e.g:

/customer?id=1,id=4,id=90
//or
/customer?age=19,company=microsoft,skill=node

this will returned a paginated result for me, I think it make sense because getByID is by ID and not by IDs, it makes sense because it use an opened filter to query by many ids, it's the filter job

dalssoft commented 3 years ago

I think express understand the /item?ids=1,2,3 different then /item/123

yes. but there should be a way to get one or multiples IDs. The REST way of doing it may differ between approaches.

I prefer /items?ids=1,2,3, but it is subjective.

Anyways, both /items/1 and /items?ids=1,2,3 should just use one UC: Find By IDs

/customer?age=19,company=microsoft,skill=node

That is another use case for search (ex: Search Customer). I don't think it should be the same use case as Find By IDs.

italojs commented 3 years ago

Anyways, both /items/1 and /items?ids=1,2,3 should just use one UC: Find By IDs

I think it will be a little bit hard to implements because:

When we implements it into getAll or search route, we don't will need to implement a middleware rule to validate if exists query and if this query only have id property, we just implements the normal flux via getAll UC /items?id=123,id=234,id=34

dalssoft commented 3 years ago

sorry, I got it wrong. the correct argument would be: both /items/1 and /items should just use one UC: Find [Entity].

todo list on herbs already do that: https://github.com/herbsjs/todolist-on-herbs/blob/master/src/infra/api/rest/routes.js#L6