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

Find Item - Naming problems and multiples IDs allowed #61

Open
dalssoft opened this issue Oct 11, 2021 · 4 comments
Open

Find Item - Naming problems and multiples IDs allowed #61

dalssoft opened this issue Oct 11, 2021 · 4 comments
Labels
enhancement New feature or request good-4-beginners hacktoberfest ready-to-work Item is ready to work on it severity-major Item is very important

Comments

@dalssoft
Copy link
Member

dalssoft commented Oct 11, 2021

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
@dalssoft dalssoft added the bug Something isn't working label Oct 11, 2021
@italojs
Copy link
Member

italojs commented Oct 12, 2021

I think express understand the /item?ids=1,2,3 different then /item/123
What do you think about we merge this feature with #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
Copy link
Member Author

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
Copy link
Member

italojs commented Oct 13, 2021

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:

  • the rest frameworks will understand it as two different routes
  • the /items route already will be used by getall usecase, we will need to add a middleware to check if it have a query, check if this query only has IDS sooo send this request to findByIds UC, I think it's will look a smell pattern/code in the API

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
Copy link
Member Author

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

@jhomarolo jhomarolo added enhancement New feature or request ready-to-work Item is ready to work on it severity-major Item is very important and removed bug Something isn't working labels Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good-4-beginners hacktoberfest ready-to-work Item is ready to work on it severity-major Item is very important
Projects
Status: Check if it is still valid
Development

No branches or pull requests

3 participants