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

Add response on fail for mysql #533

Merged
merged 19 commits into from
May 6, 2022

Conversation

G1gg1L3s
Copy link
Contributor

@G1gg1L3s G1gg1L3s commented May 4, 2022

Mysql doesn't respond to the different options of the response_on_fail (default_value, error and ciphertext) field. This PR changes that and bring support as in #521.

Checklist

G1gg1L3s added 12 commits May 3, 2022 21:51
To reuse it later in mysql
To prevent import cycles
So it looks like in the postgres version. It also simplifies and
reduces the code.
It makes sense, because:
a) It is used mostly in string context.
b) It is an identity operation only in postgres case. In mysql it
   encodes string as length encoded one.
Just copy-paste from `WithoutDefaults`
In case of EncodingError, forward the it to the client as
`QueryInterruptedError` but with custom message.
// ready to be encoded
type EncodingValue interface {
// AsPostgresBinary returns value encoded in postgres binary format
AsPostgresBinary() []byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

we use base package to store common things for all databases. what we will do after adding one more protocol? for example mysql X or mariaDB or mssql?)
okay, we want return type aware values line Int/String/Bytes which knows how to encode value according to DBs protocol. okay, there is great place for design patterns)

we can introduce fabric that can create new Int/String/Bytes values that will implement EncodingValue with methods like EncodeToBinary/EncodeToInt:

type TypeFactory interface {
  NewBytesValue(...) EncodingValue
  NewIntValue(...) EncodingValue
  NewStringValue(...) EncodingValue
}

Then update common function EncodeDefault and accept +1 parameter func EncodeDefault(..., typeFactory TypeFactory) and use it to create new Int/String values instead of calling functions NewBytesValue that should implement methods for mysql/postgresql/etc

After that target db package should implement own fabric that will return own implementations of EncodedValue and return them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right. It was one of the possible implementations, but I thought that it has much complexity. But current implementation is hard to extend. So, good idea!

@@ -14,13 +14,14 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package mysql
package base
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's mysql related functions. they should be stored in mysql package, not in base)

ValueFactory will be implemented by each backend to support
their own encoding.
This reverts commit 1a913c1.

Because as @Lagovas mentioned:
> it's mysql related functions. they should be stored in mysql package,
  not in base)
Copy link
Contributor

@Zhaars Zhaars left a comment

Choose a reason for hiding this comment

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

Lgtm

This is a temporary solution, before proper error handling lands on.
This is temporary, until we implement proper flushing of db packets.
Right now, it data is sent after our encoding error, it would probably
trigger the "unexpected sequence number" error on the client side.
"github.com/sirupsen/logrus"
)

const binaryFormat = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice)

}

testcases := []testcase{
{[]byte("string"), common.EncryptedType_String, []byte("\x06string")},
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain why we expect \x in this strings? documentation tells that binary data should be sent as hex literals like X'<hex_val>' or as 0x<hex_val>. As I remember, \x<hex_val> it is postgresql specific literals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't forget that this is a go string :)
The first byte(s) of encoded string is a length of that string. It out case, length is 6. So, to insert a 06 byte I used escaping.
Another way of writing that is:

[]byte{06, 's', 't', 'r', 'i', 'n', 'g'}

Which, imho, is less readable

Copy link
Collaborator

Choose a reason for hiding this comment

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

aaaaaaaa, understood. thanks for explanation. you r right

}

testcases := []testcase{
{[]byte("string"), common.EncryptedType_String, []byte("\x06string")},
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, why we expect \x, not 0x<hex_val>?

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

great job. especially it's pleasure to look and read to such structured commits and their separation) great example to follow)

@G1gg1L3s G1gg1L3s merged commit 077865c into master May 6, 2022
@G1gg1L3s G1gg1L3s deleted the G1gg1L3s/T2559-response-on-fail-mysql branch May 6, 2022 14:40
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.

None yet

3 participants