这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@wawhal
Copy link
Contributor

@wawhal wawhal commented Jan 17, 2019

Description

What component does this PR affect?

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Requires changes from other components? If yes, please mark the components:

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Related Issue

Solution and Design

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs update
  • Community content

Checklist:

  • I have read the contributing guide and my code conforms to the guidelines.
  • This change requires a change in the documentation.
  • I have updated the documentation accordingly.
  • I have added required tests.

@wawhal wawhal added k/enhancement New feature or improve an existing feature s/do-not-merge Do not merge this pull request to master c/console Related to console labels Jan 17, 2019
@hasura-bot
Copy link
Contributor

Review app for commit 9d9bef7 deployed to Heroku: https://hge-ci-pull-1400.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1400-9d9bef7

@hasura-bot
Copy link
Contributor

Review app for commit fcbfaf8 deployed to Heroku: https://hge-ci-pull-1400.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1400-fcbfaf8

@hasura-bot
Copy link
Contributor

Review app for commit 866c9c9 deployed to Heroku: https://hge-ci-pull-1400.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1400-866c9c9

@wawhal wawhal requested a review from praveenweb January 22, 2019 09:29
@hasura-bot
Copy link
Contributor

Review app for commit 8cd52e9 deployed to Heroku: https://hge-ci-pull-1400.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1400-8cd52e9

Copy link
Member

@praveenweb praveenweb left a comment

Choose a reason for hiding this comment

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

LGTM

@wawhal wawhal added s/ok-to-merge Status: This pull request can be merged to master and removed s/do-not-merge Do not merge this pull request to master labels Jan 22, 2019
@shahidhk shahidhk merged commit 3cfeb60 into hasura:master Jan 23, 2019
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-1400.herokuapp.com is deleted

hasura-bot pushed a commit that referenced this pull request Dec 3, 2024
… by expressions (#1400)

<!-- The PR description should answer 2 important questions: -->

### What

This PR corrects an issue where model permissions are not applied to
models used via relationships in order by expressions.

### How

The permissions are now correctly read out of the GraphQL schema
annotations and applied it to the predicate inside the relationship path
in order by targets in `crates/graphql/ir/src/order_by.rs`.

Because there are now predicates inside order by expressions, the
execution pipeline had to be updated to handle evaluating remote
predicates inside order by expressions too.

The tests in
`crates/engine/tests/execute/models/select_many/order_by/relationships/`
have been updated to test applied model permissions affecting ordering.

V3_GIT_ORIGIN_REV_ID: 783c204c44099e5319580e51e7549527d7a56245
hasura-bot pushed a commit that referenced this pull request Dec 13, 2024
…n the connector does not support it (#1443)

### What
This PR blocks the usage of nested fields and nested relationships
(where a relationship starts from a nested object) in order_by clauses
when the connector does not support it.

NDC v0.1.x has a capability for ordering by nested fields, and does not
support ordering by nested relationships at all. NDC 0.2.0 adds support
(and a capability) for ordering by nested relationships.

It also blocks being able to configure non-scalar fields as orderable in
Model v1.

These new errors are only raised if the new OpenDD flags are set. The
compatibility date to enable these flags is currently unset, pending a
production release date.

Actual support for nested relationships in ordering was "accidentally"
added in #1400, when relationship paths in order by targets were
properly modelled and implemented.

### How

Two new flags were added to OpenDD to cover the two new breaking
changes: `require_nested_support_for_order_by_expressions` and
`disallow_model_v1_ordering_non_scalar_fields`.

Logic in `crates/metadata-resolve/src/stages/models/order_by.rs` was
refactored and enhanced to check compatibility of order by expressions
used in a model against the model's source data connector.

Metadata resolve tests have been updated to test nested relationships in
ordering, and new tests added to test the new errors. Some existing
tests needed fixing to comply with the new logic without raising
warnings.

A new `execute` test has been added to cover ordering by nested
relationships in
`crates/engine/tests/execute/models/select_many/order_by/nested_relationships/`.

V3_GIT_ORIGIN_REV_ID: 3f4e01cddbbd80c4f190260df56dbdb13d0a1253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c/console Related to console k/enhancement New feature or improve an existing feature s/ok-to-merge Status: This pull request can be merged to master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants