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

fix bucket keys as ints format #590

Closed
wants to merge 4 commits into from

Conversation

Elbehery
Copy link
Member

This PR fixes the output format when bucket keys are numbers.

resolves #588

cc @ahrtr

@ahrtr
Copy link
Member

ahrtr commented Oct 26, 2023

If the data isn't an integer, the output of %d might be strange.

Currently the default output format is bytes, I suggest to only change the default format as auto.

@Elbehery Elbehery force-pushed the fix_bucket_keys_as_ints branch from b6beb26 to f0c88bb Compare October 26, 2023 16:14
@Elbehery
Copy link
Member Author

@ahrtr fixed 👍🏽

@ahrtr
Copy link
Member

ahrtr commented Oct 26, 2023

I meant to change the default format from bytes to auto,

optionsFormat := fs.String("format", "bytes", "Output format. One of: "+FORMAT_MODES+" (default: bytes)")

Please also let's go through the default format for all commands, and decide whether we should change the default format. At least, let's list them firstly.

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
@Elbehery Elbehery force-pushed the fix_bucket_keys_as_ints branch from f0c88bb to 4db9e03 Compare October 27, 2023 17:05
@Elbehery
Copy link
Member Author

@ahrtr sorry for the delay, i pushed the change now

also found that the usage was displaying incorrectly auto

--format-value=`+FORMAT_MODES+` (default: auto)

@Elbehery
Copy link
Member Author

i will go through all the commands now in a separate commit

@ahrtr
Copy link
Member

ahrtr commented Oct 27, 2023

i will go through all the commands now in a separate commit

thx, we can discuss the details on next Monday's meeting.

@Elbehery
Copy link
Member Author

found the following cmds

  • List Pages, default auto
  • PageItem, default ascii-encoded
  • Keys, default bytes
  • Get has two default, input default format is ascii-encoded, output default format is bytes

@ahrtr
Copy link
Member

ahrtr commented Oct 27, 2023

found the following cmds

  • List Pages, default auto
  • PageItem, default ascii-encoded
  • Keys, default bytes
  • Get has two default, input default format is ascii-encoded, output default format is bytes

I think we should set the default output format as "auto" for all of them. Could you also add test cases to verify the output?

bbolt/cmd/bbolt/main.go

Lines 1580 to 1587 in 2b28986

func bytesToAsciiOrHex(b []byte) string {
sb := string(b)
if isPrintable(sb) {
return sb
} else {
return hex.EncodeToString(b)
}
}

Get has two default, input default format is ascii-encoded

Suggest not to change the parse-format.

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
@Elbehery Elbehery force-pushed the fix_bucket_keys_as_ints branch from e7fbaff to 121b2b4 Compare October 27, 2023 18:33
Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
@Elbehery
Copy link
Member Author

@ahrtr

Could you also add test cases to verify the output?

The func is not exposed, and the main_test.go is in main_test package, not main package .. to add unit test will need to change either the package to main or expose the func bytesToAsciiOrHex and this will change everywhere it is used

Please let me know :)

@ahrtr
Copy link
Member

ahrtr commented Oct 27, 2023

The func is not exposed,

We don't test the function directly. Instead, we test the command, and verify whether the command's output is expected, similar to

} else if m.Stdout.String() != exp {

@@ -456,7 +456,7 @@ func (cmd *pageItemCommand) Run(args ...string) error {
fs := flag.NewFlagSet("", flag.ContinueOnError)
fs.BoolVar(&options.keyOnly, "key-only", false, "Print only the key")
fs.BoolVar(&options.valueOnly, "value-only", false, "Print only the value")
fs.StringVar(&options.format, "format", "ascii-encoded", "Output format. One of: "+FORMAT_MODES)
fs.StringVar(&options.format, "format", "auto", "Output format. One of: "+FORMAT_MODES)
Copy link
Member Author

Choose a reason for hiding this comment

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

@ahrtr Hello ✋🏽

So to test this change, I ran the TestPageItemCommand_Run and it passes.

currently the test case assert the keys and values are being inserted in

err := db.Fill([]byte("data"), 1, 100,
func(tx int, k int) []byte { return []byte(fmt.Sprintf("key_%d", k)) },
func(tx int, k int) []byte { return []byte(fmt.Sprintf("value_%d", k)) },
)
exists within the page, it capture the Main.Stdout as a string here
if !strings.Contains(m.Stdout.String(), "key_0") || !strings.Contains(m.Stdout.String(), "value_0") {

Although the key and value are being inserted are int typed, they are asserted as a string prefix

I am stuck in what should I assert to test the changes ?

I can insert hex data for instance, but still I will capture the output as a string, please correct me if I am wrong ?

for i := 0; i < 3; i++ {
key := fmt.Sprintf("%s-%d", name, i)
if err := b.Put([]byte(key), []byte{0}); err != nil {
for _, k := range []int64{10001, 10002} {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ahrtr I added this sub-test

  • insert keys in int64 format
  • asserting the keys are retrieved correctly

running this test fails on

  --- FAIL: TestKeysCommand_Run/test_keys_in_int_format (0.08s)
        btesting.go:47: Opening bbolt DB at: /var/folders/vh/wkbmyt411316pt46h81d6_300000gn/T/TestKeysCommand_Runtest_keys_in_int_format3069027896/001/db
        btesting.go:87: Closing bbolt DB at: /var/folders/vh/wkbmyt411316pt46h81d6_300000gn/T/TestKeysCommand_Runtest_keys_in_int_format3069027896/001/db
        main_test.go:341: unexpected stdout:
            
            a29c01
            a49c01

@Elbehery Elbehery force-pushed the fix_bucket_keys_as_ints branch from 08a9acd to 0e2ab60 Compare November 6, 2023 10:34
key := fmt.Sprintf("%s-%d", name, i)
val := fmt.Sprintf("val-%s-%d", name, i)
if err := b.Put([]byte(key), []byte(val)); err != nil {
for k, v := range map[string]int64{
Copy link
Member Author

Choose a reason for hiding this comment

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

same here added subtest for string keys and int64 values, similar issue

go test -run TestGetCommand_Run                                                                                                                                        ‹git:fix_bucket_keys_as_ints ✔› 11:34.33 Mon Nov 06 2023 >>> 
--- FAIL: TestGetCommand_Run (0.17s)
    --- FAIL: TestGetCommand_Run/test_values_in_int64_format (0.08s)
        btesting.go:47: Opening bbolt DB at: /var/folders/vh/wkbmyt411316pt46h81d6_300000gn/T/TestGetCommand_Runtest_values_in_int64_format893707620/001/db
        btesting.go:87: Closing bbolt DB at: /var/folders/vh/wkbmyt411316pt46h81d6_300000gn/T/TestGetCommand_Runtest_values_in_int64_format893707620/001/db
        main_test.go:422: unexpected stdout:
            
            a29c01

@Elbehery Elbehery force-pushed the fix_bucket_keys_as_ints branch from 0e2ab60 to 4be6eb8 Compare November 6, 2023 10:44
leafPageId = int(p.Id())
}
}
require.NotEqual(t, 0, leafPageId)
Copy link
Member Author

Choose a reason for hiding this comment

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

it fails here

         Error:          Should not be: 0

we sat the pageId above to 0 and it is the same as the above subtest !!!

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
@Elbehery Elbehery force-pushed the fix_bucket_keys_as_ints branch from 4be6eb8 to 436947d Compare November 6, 2023 11:02
@ahrtr
Copy link
Member

ahrtr commented Nov 6, 2023

I suggest to split this PR into three small PRs, and each of them takes care of one change for one command mentioned in #590 (comment), so as to make the review easier. I will work together with you to resolve them one by one.

What do you think?

@Elbehery
Copy link
Member Author

Elbehery commented Nov 6, 2023

#590 (comment)

on it :+1

@Elbehery
Copy link
Member Author

Elbehery commented Nov 8, 2023

@ahrtr closing this

@Elbehery Elbehery closed this Nov 8, 2023
@Elbehery Elbehery deleted the fix_bucket_keys_as_ints branch January 12, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Bbolt weirdly formatted output when listing keys of a bucket when keys are numbers rather than strings
2 participants