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

validate mutation query results #47

Merged
merged 3 commits into from
Oct 9, 2020

Conversation

ani-sha
Copy link
Member

@ani-sha ani-sha commented Oct 5, 2020

This PR contains (in reference #16) :

  • Support for GraphQL Mutations
  • Fetching corrected results
  • Support for multiple queries to issue results after mutations
  • Basic test cases

@ani-sha ani-sha requested a review from rareddy October 5, 2020 14:40
String sql = buildSQL(environment);
ctx.setSQL(sql);
LOGGER.info("SQL Executed:" + sql);

ResultSet rs = null;
Connection c = ctx.getConnection();
Statement stmt = c.createStatement();
Statement stmt = c.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,ResultSet.CONCUR_READ_ONLY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you need to set this? this is an expensive operation also not all jdbc drivers support this

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops realized that now :/
I was trying to get the no. of rows returned after query execution to validate delete operation.

Assertions.assertEquals(create_expected_mutation,create_result.get(1));
Assertions.assertEquals(create_expected_sql, create_result.get(0));

String update_query = "mutation {\n" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

split into different test cases create, update, delete

@ani-sha
Copy link
Member Author

ani-sha commented Oct 6, 2020

What is the difference between primary_key vs primary_fields? I think primary_fields was designed for this

I overlooked as we haven't used primary_fields anywhere, will take that into use.

@ani-sha
Copy link
Member Author

ani-sha commented Oct 6, 2020

In "which" situations you would make Input PK value as part of the condition? seems contradictory to me from one side, is good when a filter is not supplied, but only picking out primary fields does not sound right, and one can not select all the fields as a condition.

input would be used for a create. So should we drop the idea of using PK for conditions? Rather go along with taking and of all provided values as condition?

@ani-sha
Copy link
Member Author

ani-sha commented Oct 6, 2020

I believe primaryKey and primaryFields same here?

yes both are same.

@rareddy
Copy link
Collaborator

rareddy commented Oct 6, 2020

In "which" situations you would make Input PK value as part of the condition? seems contradictory to me from one side, is good when a filter is not supplied, but only picking out primary fields does not sound right, and one can not select all the fields as a condition.

input would be used for a create. So should we drop the idea of using PK for conditions? Rather go along with taking and of all provided values as condition?

There is no condition in the "insert" SQL command, so it should not apply if you are only using for the "create"

@ani-sha
Copy link
Member Author

ani-sha commented Oct 6, 2020

I think we need to allow the "Address" without PK, because that is possible in a relational database. There will be one to many cases where many side just there because of the other side. Here Address means nothing without a customer. That means the SSN is not null field than it has it's own primary key. What we can do is any table that does not have a primary key, we should not have the top level methods like address, createAddress, updateAddress and deleteAddress then we can handle them later with nested Customer.

I thought defining a PK was mandatory.

In "which" situations you would make Input PK value as part of the condition? seems contradictory to me from one side, is good when a filter is not supplied, but only picking out primary fields does not sound right, and one can not select all the fields as a condition.

input would be used for a create. So should we drop the idea of using PK for conditions? Rather go along with taking and of all provided values as condition?

There is no condition in the "insert" SQL command, so it should not apply if you are only using for the "create"

After an insert, the select query uses the input PK value to define the condition.

@rareddy
Copy link
Collaborator

rareddy commented Oct 6, 2020

I thought defining a PK was mandatory.

It is not mandatory, the implementation can choose to say we only want to support tables with PK, but that would be wrong IMO as we would not able to make them change the DDL for their databases. We just need to change how we are going to support that case.

After an insert, the select query uses the input PK value to define the condition.

By not allowing the top-level queries (only customer, not address) this case will be still true, As Customer will have the PK.

@ani-sha
Copy link
Member Author

ani-sha commented Oct 6, 2020

I thought defining a PK was mandatory.

It is not mandatory, the implementation can choose to say we only want to support tables with PK, but that would be wrong IMO as we would not able to make them change the DDL for their databases. We just need to change how we are going to support that case.

After an insert, the select query uses the input PK value to define the condition.

By not allowing the top-level queries (only customer, not address) this case will be still true, As Customer will have the PK.

Well in that case i will remove the PK from address, and need to do some research about correct the way to approach for multiple queries.

Copy link
Collaborator

@rareddy rareddy left a comment

Choose a reason for hiding this comment

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

LGTM

@rareddy rareddy merged commit 73615e2 into graphqlcrud:master Oct 9, 2020
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.

2 participants