Skip to content

Return generic error messages in production mode#1357

Merged
severussundar merged 44 commits intomainfrom
dev/shyamsundarj/genericize-error-messages-bad-requests
Apr 2, 2023
Merged

Return generic error messages in production mode#1357
severussundar merged 44 commits intomainfrom
dev/shyamsundarj/genericize-error-messages-bad-requests

Conversation

@severussundar
Copy link
Contributor

@severussundar severussundar commented Mar 24, 2023

Why make this change?

  • Closes Genericize error messages between bad procedure and table/view requests #671
    • Consider a GET request such as : api/Book/id/one.
      The parameter type for id should be an int. In such a case, the error message returned is the following
      "Parameter \"one\" cannot be resolved as column \"id\" with type \"Int32\".",
      While this error message is helpful, it reveals more information than necessary. For example, it can be deduced from the error message that the database object backing the entity Book is a table and the system type for the column id is Int32
    • To not reveal additional information, a generic error message: Invalid value provided for field: id will be returned when running in Production mode. When running in Development mode, the detailed error message will be returned.
    • When parameters/arguements are provided with incorrect type values in the graphQL requests, a generic error message : The specified argument value does not match the argument type is returned irrespective of the host mode being Development or Production.

What is this change?

  • A new method IsDevelopmentMode() is introduced in the ISqlMetadataProvider interface and implementations for this method is provided by the implementing classes.
  • Exceptions are constructed with different error messages for Production and Development host modes.
  • Tests are added only for SQL database types as the error messages for REST requests reveal additional information about the entities. GraphQL requests return a generic error when running in both Development and Production mode. So, no tests are added for Cosmos.

Additional Refactoring Changes

  • A common method GetParamAsSystemType is introduced which casts the parameter values for tables, views and stored-procedures to the respective column/parameter system types. Separate methods for such casting for tables, views and stored-procedures is refactored to a single common method.

How was this tested?

  • Integration Tests
  • Manually

Sample requests

Error Message when running in Development mode:

image

Error Message when running in Production mode:

image

@severussundar
Copy link
Contributor Author

severussundar commented Mar 30, 2023

                // This case should not arise. We have issue for this to handle nullable type columns. Issue #146.

Just curious, was this issue resolved now? If yes, does this need to be modified in any way - doesnt have to be part of this PR - but wondering if we need to fix this.


Refers to: src/Service/Resolvers/Sql Query Structures/SqlQueryStructure.cs:520 in 4f947e5. [](commit_id = 4f947e5, deletion_comment = False)


This was resolved by PR #324. I'll pick up the refactoring changes to remove code paths that don't get executed in a follow-up PR. I've created this issue to track

Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving few comments to address, LGTM otherwise.

@Aniruddh25
Copy link
Collaborator

Aniruddh25 commented Mar 31, 2023

                message: $"{columnName} is not a valid column of {DatabaseObject.Name}",

Why are we using the same message here irrespective development mode?


Refers to: src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs:126 in 4f947e5. [](commit_id = 4f947e5, deletion_comment = False)

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left few nits. Thanks for refactoring!

@severussundar
Copy link
Contributor Author

severussundar commented Apr 1, 2023

                message: $"{columnName} is not a valid column of {DatabaseObject.Name}",

Why are we using the same message here irrespective development mode?

Refers to: src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs:126 in 4f947e5. [](commit_id = 4f947e5, deletion_comment = False)

In BaseSqlQueryStructure.GetColumnSystemType(), that code block never gets executed.

REST Requests:

If an invalid column name is used in the requests, it gets caught in RequestValidator.ValidateRequestContext() for REST requests. The execution does not reach this point. Both in development and production modes, the following error message is returned.

Request: https://localhost:5001/api/Book/id/1?$select=id,hello

Error Message: Invalid field to be returned requested: hello

GraphQL requests:

Incase of graphQL requests, hotchocolate catches this and returns the following error message when running in both development and production modes.
Error message: The field `hello` does not exist on the type `book`


The code flow in BaseSqlQueryStructure.GetColumnSystemType() where the exception is thrown can be removed which I'll take up in issue #1387

@severussundar severussundar merged commit e848b46 into main Apr 2, 2023
@severussundar severussundar deleted the dev/shyamsundarj/genericize-error-messages-bad-requests branch April 2, 2023 14:56
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.

Genericize error messages between bad procedure and table/view requests

4 participants