### Motivation
While we strive to write clear code, we have historically struggled at Hasura with having very different styles and standards across the codebase. There's been efforts to standardize our coding style, we have an official styleguide that isn't maintained as closely as it should... We still have some work in front of us.
However, in the last ~year or so, there's been a huge push towards incrementally improving the situation. As part of this we've been blocking PRs that don't add enough comments, or don't improve the files that they touch.
While looking at `Hasura.GraphQL.Analyse`, it became apparent that this file did not meet the engineering standards that I would expect to see addressed during a code review. Some ways in which I think it falls short:
- lack of documentation
- no clear distinction between public / internal components
- "unidiomatic" Haskell code (such as using `Either Result Error`)
While there's no problem with a file looking like this during development, those issues should have been caught at review time. The fact that they weren't indicates a problem in our process that we will need to address: code quality and maintainability is paramount, and we all need to do our part.
### Description
This PR rewrites all of `Hasura.GraphQL.Analyze`, and adapts `Hasura.Server.OpenAPI` accordingly where needed. I've attempted to clarify names and add documentation based on my understanding of the code, and to clean what was unused (such as field variables). I don't think this PR is good enough as is, and I welcome criticism where I got my comments wrong / am happy to help y'all add more.
This PR makes one small change in the way error messages are reported (and adjusts the corresponding test accordingly); each error message is now prefixed with the path within the selection set:
```
⚠️ $.test.foo.bar.baz.mizpelled: field 'mizpelled' not found in object 'Baz'
```
### Note
This PR is currently **on top of #3962**. You can preview the changes in isolation by [diffing the branches](https://github.com/hasura/graphql-engine-mono/compare/nicuveo/clean-rest-endpoint-inconsistency-check..nicuveo/rewrite-analysis).
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3963
Co-authored-by: paritosh-08 <85472423+paritosh-08@users.noreply.github.com>
GitOrigin-RevId: 5ec38e0e753f0c12096a350db0737658495e2f15
### Description
This small PR improves the representation of an endpoint method from `Text` to an enum of the supported methods. Additionally, it cleans some of the instances defined on surrounding types (such as Postgres-specific instances on Endpoint types).
Due to a name conflict, this makes `RQL.Types.Endpoint` impossible to re-export from `RQL.Types`, which in turn forces several other modules to import it explicitly, which I think is fine since we want to ultimately get rid of `RQL.Types`.
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3965
GitOrigin-RevId: 33869007d0d818ddf486fb61d1f6099f9dad7570
spec: https://github.com/hasura/graphql-engine-mono/pull/2278
Briefly:
- extend metadata so that allowlist entries get a new scope field
- update `add_collection_to_allowlist` to accept this new scope field,
and adds `update_scope_of_collection_in_allowlist` to change the scope
- scope can be global or role-based; a collection is available for every
role if it is global, and available to every listed role if it is role-based
- graphql-engine-oss is aware of role-based allowlist metadata; collections
with non-global scope are treated as if they weren't in the allowlist
To run the tests:
- `cabal run graphql-engine-tests -- unit --match Allowlist`
- py-tests against pro:
- launch `graphql-engine-pro` with `HASURA_GRAPHQL_ADMIN_SECRET` and `HASURA_GRAPHQL_ENABLE_ALLOWLIST`
- `pytest test_allowlist_queries.py --hge-urls=... --pg-urls=... --hge-key=... --test-allowlist-queries --pro-tests`
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/2477
Co-authored-by: Anon Ray <616387+ecthiender@users.noreply.github.com>
Co-authored-by: Robert <132113+robx@users.noreply.github.com>
GitOrigin-RevId: 01f8026fbe59d8701e2de30986511a452fce1a99
### Description
The GraphQL spec has to conflicting requirements:
1. an object must contain at least one field: the schema may not contain empty objects
2. the _query_root_ must always be present
Given _1_, the schema generation code removes from the schema all fields that would result in empty objects, such as a table for which a user does not have select permissions. But, as a result, our code also potentially removes _query_root_ if it is empty, breaking _2_.
This PR introduces a dummy "placeholder" field in the query root if it's empty, to ensure we never remove it from the schema.
### Remaining work
- [x] changelog entry
- [x] tests
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/148
GitOrigin-RevId: bfd6bfcc2f3de92900b6ba566012f093399ca037
## Description
When setting up a remote relationship to a remote schema, values coming from the left-hand side are given as _arguments_ to the targeted field of the remote schema. In turn, that means we need to adjust the arguments to that remote field; in the case of input objects, it means creating a brand new input object in which the relevant fields have been removed.
To both avoid conflicts, and be explicit, we give a pretty verbose name to such an input object: its original name, followed by "remote_rel", followed by the full name of the field (table name + relationship name). The bug there was introduced when working on extending remote relationships to other backends: we changed the code that translates the table name to a graphql identifier to be generic, and use the table's `ToTxt` instance instead. However, when a table is not in the default schema, the character used by that instance is `.`, which is not a valid GraphQL name.
This PR fixes it, by doing two things:
- it defines a safe function to translate LHS identifiers to graphql names (by replacing all invalid characters by `_`)
- it doesn't use `unsafeMkName` anymore, and checks at validation time that the type name is correct
## Further work
On this PR:
- [x] add a test
- [x] write a Changelog entry
Beyond this PR, we might want to:
- prioritize #1747
- analyze all calls to `unsafeMkName` and remove as many as possible
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3363
GitOrigin-RevId: fe98eb1d34157b2c8323af453f5c369de616af38
## Description
I come across a flaky test due to inconsistent error messages from the odbc lib we use for MSSQL database interactions.
```
cabal new-run -- test:graphql-engine-tests mssql
Up to date
Database.MSSQL.TransactionSpec
runTx
runs command in a transaction
commits a successful transaction, returning a single field
commits a successful transaction, returning multiple fields
an unsuccesful transaction, expecting Int
a successfull query expecting multiple rows
an unsuccesful transaction; expecting single row
displays the SQL Server error on an unsuccessful transaction FAILED [1]
rolls back an unsuccessful transaction
Failures:
src-test/Database/MSSQL/TransactionSpec.hs:60:15:
1) Database.MSSQL.TransactionSpec.runTx displays the SQL Server error on an unsuccessful transaction
expected: UnsuccessfulReturnCode "odbc_SQLExecDirectW" (-1) "[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The definition for column 'INVALID_SYNTAX' must include a data type.[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The definition for column 'INVALID_SYNTAX' must include a data type."
but got: UnsuccessfulReturnCode "odbc_SQLExecDirectW" (-1) "[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The definition for column 'INVALID_SYNTAX' must include a data type.[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The definition for column 'INVALID_SYNTAX' must include a data type.\DEL"
To rerun use: --match "/Database.MSSQL.TransactionSpec/runTx/displays the SQL Server error on an unsuccessful transaction/"
Randomized with seed 1101559172
Finished in 0.2140 seconds
8 examples, 1 failure
```
From above, we got a error message with `\DEL` appended. It is also driving the tests to fail in the CI on random PRs.
We brought this into notice of "fpco", the authors of the library and they got us a [quick fix](https://github.com/fpco/odbc/pull/43), which also improves the errors by removing the redundancy of the error message.
In this PR
- We update the `odbc` library git reference to fc5b592a60
- Update the error messages in tests to conform with improved error messages from `odbc`
## Related issues
Closes https://github.com/hasura/graphql-engine-mono/issues/3340
## Changelog
- ✅ `CHANGELOG.md` is updated with user-facing content relevant to this PR.
## Affected components
- ✅ server
- ✅ tests
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3345
GitOrigin-RevId: a5694e8afb58b5ad71b9c9635a80dea1ec449f51
