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

refactor: drop check_identity from ExecutionSpec #1414

Closed
tusharmath opened this issue Mar 13, 2024 · 16 comments
Closed

refactor: drop check_identity from ExecutionSpec #1414

tusharmath opened this issue Mar 13, 2024 · 16 comments
Labels
💎 Bounty state: inactive No current action needed/possible; issue fixed, out of scope, or superseded.

Comments

@tusharmath
Copy link
Contributor

CheckIdentity should be enabled for all specifications, we don't need this configuration in ExecutionSpec.

@tusharmath
Copy link
Contributor Author

/bounty 100

Copy link

algora-pbc bot commented Mar 13, 2024

## 💎 $100 bounty • Tailcall Inc.

### Steps to solve:
1. Start working: Comment /attempt #1414 with your implementation plan
2. Submit work: Create a pull request including /claim #1414 in the PR body to claim the bounty
3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

🙏 Thank you for contributing to tailcallhq/tailcall!
🧐 Checkout our guidelines before you get started.
💵 More about our bounty program.

Attempt Started (GMT+0) Solution
🔴 @35C4n0r Mar 13, 2024, 10:36:34 AM WIP
🟢 @ssddOnTop Apr 14, 2024, 4:43:47 PM WIP

@35C4n0r
Copy link
Contributor

35C4n0r commented Mar 13, 2024

I would like to try it.

@35C4n0r
Copy link
Contributor

35C4n0r commented Mar 13, 2024

@tusharmath tests fail on removing it from the execution_spec.rs and the .md files.
Where are the rules to these identity checks.

SUPER_IMPOPRTANAT_IDENTITY: 
schema @server @upstream {
  query: Query
}

type Query @addField(name: "username", path: ["users", "0", "name"]) {
  users: [User] @http(baseURL: "http://jsonplaceholder.typicode.com", path: "/users")
}

type User {
  name: String
}
SUPER_IMPOPRTANAT_CONTENT: 
schema {
  query: Query
}

type User {
  name: String
}

type Query @addField(name: "username", path: ["users", "0", "name"]) {
  users: [User] @http(path: "/users", baseURL: "http://jsonplaceholder.typicode.com")
}
Diff < left / right > :
<schema {
>schema @server @upstream {
   query: Query
 }
 
<type User {
<  name: String
>type Query @addField(name: "username", path: ["users", "0", "name"]) {
>  users: [User] @http(baseURL: "http://jsonplaceholder.typicode.com", path: "/users")
 }
 
<type Query @addField(name: "username", path: ["users", "0", "name"]) {
<  users: [User] @http(path: "/users", baseURL: "http://jsonplaceholder.typicode.com")
>type User {
>  name: String
 }

@35C4n0r
Copy link
Contributor

35C4n0r commented Mar 13, 2024

/attempt #1414

Algora profile Completed bounties Tech Active attempts Options
@35C4n0r    3 tailcallhq bounties
+ 6 bounties from 2 projects
Python, JavaScript,
HTML & more
Cancel attempt

Copy link

algora-pbc bot commented Mar 13, 2024

@35C4n0r: You're good to go!

@tusharmath
Copy link
Contributor Author

How many of such tests fail? Ideally they shouldn't fail.

@35C4n0r
Copy link
Contributor

35C4n0r commented Mar 13, 2024

@35C4n0r
Copy link
Contributor

35C4n0r commented Mar 14, 2024

@tusharmath the tests fail, in all of them (graphql and json server) there are either changes in the order of the parameters, methods or type declarations + schema --> schema @server @upstream, ig this is becuse these tests were not written with identity check in mind, should I try to create a script which fixes the test mds

@ssddOnTop
Copy link
Member

@35C4n0r are you still on it?

@35C4n0r
Copy link
Contributor

35C4n0r commented Apr 14, 2024

@ssddOnTop Sure, feel free to use the code here e2e3e37#diff-e47f835532ae836901521cecc2e77863f98286e977c1920b9ca545a9d4291aa0
There was one issue that was Empty directives still remained on types, maybe I'm doing something wrong.
I'll cancel my attempt.

@35C4n0r 35C4n0r removed their assignment Apr 14, 2024
@ssddOnTop
Copy link
Member

ssddOnTop commented Apr 14, 2024

/attempt

Algora profile Completed bounties Tech Active attempts Options
@ssddOnTop 37 tailcallhq bounties
Rust, Java,
C & more
Cancel attempt

@ssddOnTop
Copy link
Member

ssddOnTop commented May 25, 2024

closing my PR for now, will be doing after #2026

Copy link

algora-pbc bot commented Jul 19, 2024

Here are some steps and pointers to help you get started on resolving this issue:

  1. Locate and Remove check_identity from ExecutionSpec Definition:

    • Identify where ExecutionSpec is defined and remove the check_identity field from its definition.
    • Example file paths where ExecutionSpec is referenced:
      • /tests/execution_spec.rs
      • /tests/snapshots/execution_spec__batching-disabled.md_assert_0.snap
      • /tests/snapshots/execution_spec__batching.md_assert_0.snap
      • /tests/snapshots/execution_spec__test-batching-group-by.md_client.snap
      • /tests/snapshots/execution_spec__call-operator.md_assert_12.snap
      • /tests/snapshots/execution_spec__caching.md_assert_0.snap
      • /tests/snapshots/execution_spec__batching-group-by-default.md_assert_0.snap
      • /tests/snapshots/execution_spec__graphql-dataloader-no-batch-request.md_assert_0.snap
      • /tests/snapshots/execution_spec__grpc-override-url-from-upstream.md_assert_0.snap
      • /tests/snapshots/execution_spec__with-args-url.md_client.snap
      • /tests/snapshots/execution_spec__resolved-by-parent.md_assert_0.snap
  2. Update Tests and Snapshots:

    • Ensure that all tests and snapshot files that reference check_identity are updated to reflect its removal.
    • Example test files:
      • /tests/snapshots/execution_spec__batching-disabled.md_assert_0.snap
      • /tests/snapshots/execution_spec__batching.md_assert_0.snap
      • /tests/snapshots/execution_spec__test-batching-group-by.md_client.snap
      • /tests/snapshots/execution_spec__call-operator.md_assert_12.snap
      • /tests/snapshots/execution_spec__caching.md_assert_0.snap
      • /tests/snapshots/execution_spec__batching-group-by-default.md_assert_0.snap
      • /tests/snapshots/execution_spec__graphql-dataloader-no-batch-request.md_assert_0.snap
      • /tests/snapshots/execution_spec__grpc-override-url-from-upstream.md_assert_0.snap
      • /tests/snapshots/execution_spec__with-args-url.md_client.snap
      • /tests/snapshots/execution_spec__resolved-by-parent.md_assert_0.snap
  3. Refactor Code to Assume check_identity is Always Enabled:

    • Modify any logic that conditionally checks check_identity to assume it is always enabled.
    • Ensure that the removal does not introduce any security or stability issues.
  4. Run Tests to Ensure Stability:

    • After making the changes, run the entire test suite to ensure that no existing functionality is broken.
    • Pay special attention to tests related to identity checks to confirm they still pass.
  5. Update Documentation:

    • If there is any documentation that mentions check_identity, update it to reflect the new behavior.
    • Example documentation file:
      • /tests/README.md

Copy link

Action required: Issue inactive for 30 days.
Status update or closure in 7 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 18, 2024
Copy link

Issue closed after 7 days of inactivity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💎 Bounty state: inactive No current action needed/possible; issue fixed, out of scope, or superseded.
Projects
None yet
3 participants