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

Fix a modelFactory bug #459

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Fix a modelFactory bug #459

merged 1 commit into from
Mar 18, 2024

Conversation

T-vK
Copy link
Contributor

@T-vK T-vK commented Mar 12, 2024

I found another bug regarding the modelFactory. Many places in the code were exclusively using the initially provided model instead of the model dynamically provided through modelfactory.getModel.

As we don't have any integration tests for this feature, I created a little test script to verify that it works as expected:

var express = require('express')
var bodyParser = require('body-parser')
var methodOverride = require('method-override')
var mongoose = require('mongoose')
var restify = require('express-restify-mongoose')
const request = require('request-promise')
var app = express()
var router = express.Router()

app.use(bodyParser.json())
app.use(methodOverride())

mongoose.connect('mongodb://localhost/database')

var customerSchema = new mongoose.Schema({
  name: { type: String, required: true },
  comment: { type: String }
})
var userSchema = new mongoose.Schema({
  name: { type: String },
  email: { type: String, required: true }
})

var customerModel = mongoose.model('Customer', customerSchema)
var userModel = mongoose.model('User', userSchema)

restify.serve(router, customerModel, {
  modelFactory: {
    getModel: async (req) => {
      return null // THIS IS EXPECTED TO FAIL
    }
  }
})

restify.serve(router, userModel, {
  modelFactory: {
    getModel: async (req) => {
      return userModel // THIS IS EXPECTED TO WORK
    }
  }
})

app.use(router)

app.listen(3000, async () => {
  try {
    // GET http://localhost:3000/api/v1/User should work
    await testEndpoint('http://localhost:3000/api/v1/User', 200);
    // GET http://localhost:3000/api/v1/Customer should return an error
    await testEndpoint('http://localhost:3000/api/v1/Customer', 400);
    console.log("Success!")
    process.exit(0)
  } catch (e) {
    console.log(e)
    console.log("Failed!")
    process.exit(1)
  }
})

async function testEndpoint(url, expectedStatus) {
  let res = null
  try {
    res = await request({url, resolveWithFullResponse: true})
  } catch (error) {
    res = error
  }
  if (res.statusCode !== expectedStatus)
    throw new Error(`${url} did not return expected status code. Got "${res.statusCode}" instead of "${expectedStatus} - Error: ${res.error}"`)
  else
    console.log(`${url} is behaving as expected.`)
}

We should turn this into an integration test at some point to avoid bugs like this in the future.

@T-vK
Copy link
Contributor Author

T-vK commented Mar 13, 2024

I just made another tiny adjustment to this PR to improve the performance a bit by registering the middleware for the model retrieval on the ERM routes directly (instead of as a global middleware for all routes).

@florianholzapfel florianholzapfel merged commit 86e5447 into florianholzapfel:main Mar 18, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants