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

Has one relation error (MODEL_NOT_FOUND) #4278

Closed
sujeshthekkepatt opened this issue Nov 8, 2019 · 12 comments
Closed

Has one relation error (MODEL_NOT_FOUND) #4278

sujeshthekkepatt opened this issue Nov 8, 2019 · 12 comments

Comments

@sujeshthekkepatt
Copy link
Contributor

I have two models usermodal,my_another_model. usermodel has a hasOne relationship with my_another_model. Whenever I access the given endpoint I get the following error. This happens only when the my_another_model does not have any data. The issue happens when I GET the endpoint /usermodel/me/myAnotherModel

Endpoint

/usermodel/me/myAnotherModel

usermodel

{
  "name": "usermodel",
  "base": "User",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "mixins": {
   
  },
  "properties": {
    "first_name": {
      "type": "string",
      "required": true
    },
    "last_name": {
      "type": "string",
      "required": true
    },
  },
  "validations": [],
  "relations": {
    "myAnotherModel": {
      "type": "hasOne",
      "model": "my_another_model",
      "foreignKey": "",
      "options": {
        "nestRemoting": true
      }
    }
  },
  "acls": [
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "ALLOW"
    },

  ],
  "methods": {
}

}

my_another_model

{
  "name": "my_another_model",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {
    "name": {
      "type": "string",
      "required": true
    },
    "phone": {
      "type": "string"
    },
    "email": {
      "type": "string"
    }
  },
  "validations": [],
  "relations": {
    "usermodel": {
      "type": "belongsTo",
      "model": "usermodel",
      "foreignKey": ""
    }
  },
  "acls": [],
  "methods": {}
}

Error Message

I get the following error message,

   Error: Unknown "my_another_model" id "undefined".```
Any help appreciated.
@bajtos
Copy link
Member

bajtos commented Nov 15, 2019

@sujeshthekkepatt thank you for reporting the issue. Please create a small app we can run to reproduce the issue, see https://loopback.io/doc/en/contrib/Reporting-issues.html#loopback-3x-bugs

@sujeshthekkepatt
Copy link
Contributor Author

sujeshthekkepatt commented Nov 16, 2019

@bajtos I have added the necessary code for reproduction. The forked repo can be found here.

Steps for reproduction

  • Run the cloned repo.
  • create(post) a usermodel instance using the endpoint /usermodels
  • create(post) a /usermodels/{id}/myanothermodels. Here me can be used instead of id.
  • Retrieve (get) the related instance using /usermodels/{id}/myanothermodels. Here also me can be used instead of id. Now we can retrieve the instance.
  • Now delete the myanothermodels instance using the endpoint /usermodels/{id}/myanothermodels.
  • Now try to retrieve instance using /usermodels/{id}/myanothermodels we get the following error { "error": { "statusCode": 404, "name": "Error", "message": "Unknown \"myanothermodel\" id \"undefined\".", "code": "MODEL_NOT_FOUND" } }

@sujeshthekkepatt
Copy link
Contributor Author

@bajtos Any updates ?

@bajtos
Copy link
Member

bajtos commented Nov 22, 2019

create(post) a /usermodels/{id}/myanothermodels. Here me can be used instead of id.

Just for the information: it's necessary to login and set the access token in API Explorer first, before calling this endpoint.

Other steps work as described and I did receive the same error as you.

@bajtos
Copy link
Member

bajtos commented Nov 22, 2019

I am also able to reproduce the problem when I call GET /usermodels/{id}/myanothermodels right after creating the user and logging in, it's not necessary to create and delete myanothermodel.

@bajtos
Copy link
Member

bajtos commented Nov 22, 2019

The weird part is that we have a test in loopback to verify that 404 Not Found error is returned when the target model of HasOne relation was not found, see here:

it('should not find the referenced model', function(done) {
const url = '/api/customers/' + cust.id + '/profile';
this.get(url)
.expect(404, function(err, res) {
done(err);
});
});

Model definitions:

Oh! I think I see the problem now. The application correctly returns HTTP status code 404, but the error message is misleading.

{
  "error": {
    "statusCode": 404,
    "name": "Error",
    "message": "Unknown \"myanothermodel\" id \"undefined\".",
    "code": "MODEL_NOT_FOUND"
  }
}

@sujeshthekkepatt could you please confirm you are seeing 404 status code too?

What happens under the hood: LoopBack's REST API layer calls juggler's method to retrieve the target model of HasOne relation. This methods returns null because no target was found. LoopBack's REST API layer converts null to 404 Not Found response.

Here is the relevant code:

loopback/lib/model.js

Lines 564 to 573 in 2db09a3

function convertNullToNotFoundError(toModelName, ctx, cb) {
if (ctx.result !== null) return cb();
const fk = ctx.getArgByName('fk');
const msg = g.f('Unknown "%s" id "%s".', toModelName, fk);
const error = new Error(msg);
error.statusCode = error.status = 404;
error.code = 'MODEL_NOT_FOUND';
cb(error);
}

Maybe we can improve the error message as follows?

    var fk = ctx.getArgByName('fk');
    var msg = fk
      ? g.f('Unknown "%s" id "%s".', toModelName, fk)
      : g.f('No "%s" instance(s) found', toModelName);
    var error = new Error(msg);

With this change in place, I am getting the following HTTP response:

{
  "error": {
    "statusCode": 404,
    "name": "Error",
    "message": "No \"myanothermodel\" instance(s) found",
    "code": "MODEL_NOT_FOUND"
  }
}

Thoughts?

@sujeshthekkepatt
Copy link
Contributor Author

sujeshthekkepatt commented Nov 23, 2019

@bajtos yes I am also getting 404. Yep the current error message is not a proper one. But returning null in this scenario is a recommended one,right ? So that we could easily check for null value and continue with our logic. But throwing error makes it bit confusing. And it introduces unnecessary error handling. Error message improvement helps to understand what happened under the hood properly but not helping to write any efficient code. So I think returning an object when there is data and not returning any object when there is no data corresponding to the target model like we do with other operations is best in this scenario. What do you think @bajtos ??

@sujeshthekkepatt
Copy link
Contributor Author

Any update on this @bajtos ?

@bajtos
Copy link
Member

bajtos commented Nov 29, 2019

The current behavior is intentional. The 404 error object is created only when you call the endpoint via REST API. Without this interception, your HTTP clients will get 204 No Content response. IMO, 404 Not Found is more appropriate - it better describes what happened.

@sujeshthekkepatt
Copy link
Contributor Author

sujeshthekkepatt commented Nov 29, 2019

@bajtos In that case as you suggested, improving the error message seems a perfect fit. May be I can give a PR .

@bajtos
Copy link
Member

bajtos commented Nov 29, 2019

In that case as you suggested, improving the error message seems a perfect fit. May be I can give a PR .

Great! See our Contributing guide and Submitting a pull request to LoopBack 4 to get started.

@stale
Copy link

stale bot commented Jan 28, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants