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.

Sign up for GitHub

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: change pageItemCommand default format auto #596

Merged
merged 1 commit into from
Nov 7, 2023
Merged

fix: change pageItemCommand default format auto #596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/bbolt/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
fs.BoolVar(&options.help, "h", false, "")
if err := fs.Parse(args); err != nil {
return err
Expand Down Expand Up @@ -606,7 +606,7 @@ Additional options include:
--value-only
Print only the value
--format
Output format. One of: `+FORMAT_MODES+` (default=ascii-encoded)
Output format. One of: `+FORMAT_MODES+` (default=auto)

page-item prints a page item key and value.
`, "\n")
Expand Down
104 changes: 76 additions & 28 deletions cmd/bbolt/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
crypto "crypto/rand"
"encoding/binary"
"encoding/hex"
"fmt"
"io"
"math/rand"
Expand All @@ -14,12 +15,12 @@ import (
"testing"

"go.etcd.io/bbolt/internal/btesting"
"go.etcd.io/bbolt/internal/guts_cli"

"github.com/stretchr/testify/require"

bolt "go.etcd.io/bbolt"
main "go.etcd.io/bbolt/cmd/bbolt"
"go.etcd.io/bbolt/internal/guts_cli"
)

// Ensure the "info" command can print information about a database.
Expand Down Expand Up @@ -135,35 +136,76 @@ func TestPageCommand_Run(t *testing.T) {
}

func TestPageItemCommand_Run(t *testing.T) {
db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: 4096})
srcPath := db.Path()

// Insert some sample data
t.Log("Insert some sample data")
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)) },
)
require.NoError(t, err)

defer requireDBNoChange(t, dbData(t, srcPath), srcPath)
testCases := []struct {
name string
printable bool
itemId string
expectedKey string
expectedValue string
}{
{
name: "printable items",
printable: true,
itemId: "0",
expectedKey: "key_0",
expectedValue: "value_0",
},
{
name: "non printable items",
printable: false,
itemId: "0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Q : why we use the same itemId as the printable item above ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good question. You can even check all items using a for loop,

for i:=0; i<100; i++ {
    // check $i item
}

Copy link
Member Author

Choose a reason for hiding this comment

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

but we are using only one itemId for all the data, no ?

expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),
expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),

Why we use key as 0+1 and value as 0+2 ?

expectedValue: hex.EncodeToString(convertInt64IntoBytes(0 + 2)),
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: 4096})
srcPath := db.Path()

t.Log("Insert some sample data")
err := db.Update(func(tx *bolt.Tx) error {
b, bErr := tx.CreateBucketIfNotExists([]byte("data"))
if bErr != nil {
return bErr
}

meta := readMetaPage(t, srcPath)
leafPageId := 0
for i := 2; i < int(meta.Pgid()); i++ {
p, _, err := guts_cli.ReadPage(srcPath, uint64(i))
require.NoError(t, err)
if p.IsLeafPage() && p.Count() > 1 {
leafPageId = int(p.Id())
}
}
require.NotEqual(t, 0, leafPageId)
for i := 0; i < 100; i++ {
if tc.printable {
if bErr = b.Put([]byte(fmt.Sprintf("key_%d", i)), []byte(fmt.Sprintf("value_%d", i))); bErr != nil {
return bErr
}
} else {
k, v := convertInt64IntoBytes(int64(i+1)), convertInt64IntoBytes(int64(i+2))
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, why we increment the i ? .. not the case in the printable data above !

if bErr = b.Put(k, v); bErr != nil {
return bErr
}
}
}
return nil
})
require.NoError(t, err)
defer requireDBNoChange(t, dbData(t, srcPath), srcPath)
Copy link
Member Author

Choose a reason for hiding this comment

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

i wonder, why the database should not change, since we have inserted sample data ?

Copy link
Member

Choose a reason for hiding this comment

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

Note that purpose of the requireDBNoChange is to ensure the command doesn't change the db's data.

Copy link
Member Author

Choose a reason for hiding this comment

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

so dbData read the database file that is already on-disk .. then requireDBNoChange will read the new data file after the test data has been inserted in the transaction above ..

My question is, how the database should not change, while we are inserting sample data ?

please correct me if i am wrong

Copy link
Member

Choose a reason for hiding this comment

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

When a "defer" statement gets executed, its value and parameters to the call are evaluated as usual and saved anew, so dbData(t, srcPath) is executed right away. It reads current db's data. Note that the we have already finished inserting all data at the moment.

But the actual function is not invoked until the surrounding function returns. When it gets really executed, it reads the db's data again, and compare it with the previous data read in dbData(t, srcPath).

Copy link
Member

Choose a reason for hiding this comment

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

The rough workflow:

  1. insert some sample data
  2. read the db's data using dbData(t, srcPath)
  3. execute the command (the main part of the test);
  4. read the db's data again, and compare it with the data read at step 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, makes perfect sense now :) .. thanks tonsss 🙏🏽


meta := readMetaPage(t, srcPath)
leafPageId := 0
for i := 2; i < int(meta.Pgid()); i++ {
p, _, err := guts_cli.ReadPage(srcPath, uint64(i))
require.NoError(t, err)
if p.IsLeafPage() && p.Count() > 1 {
leafPageId = int(p.Id())
}
}
require.NotEqual(t, 0, leafPageId)

m := NewMain()
err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), "0")
require.NoError(t, err)
if !strings.Contains(m.Stdout.String(), "key_0") || !strings.Contains(m.Stdout.String(), "value_0") {
t.Fatalf("Unexpected output:\n%s\n", m.Stdout.String())
m := NewMain()
err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), tc.itemId)
require.NoError(t, err)
if !strings.Contains(m.Stdout.String(), tc.expectedKey) || !strings.Contains(m.Stdout.String(), tc.expectedValue) {
t.Fatalf("Unexpected output:\n%s\n", m.Stdout.String())
}
Comment on lines +202 to +207
Copy link
Member

@ahrtr ahrtr Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you want, you can also enhance the test case something like below,

	for i:=0; i<100; i++ {
		m := NewMain()
		err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), fmt.Sprintf("%d", i))
		require.NoError(t, err)
		if !strings.Contains(m.Stdout.String(), xxx) || !strings.Contains(m.Stdout.String(), yyy) {
			t.Fatalf("Unexpected output:\n%s\n", m.Stdout.String())
		}
	}

Then you can remove

		itemId        string
		expectedKey   string
		expectedValue string

Copy link
Member Author

Choose a reason for hiding this comment

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

i like the anonymous struct style more, because it is easier to be read by others.. thanks for your answer 🙏🏽

})
}
}

Expand Down Expand Up @@ -629,3 +671,9 @@ func requireDBNoChange(t *testing.T, oldData []byte, filePath string) {
noChange := bytes.Equal(oldData, newData)
require.True(t, noChange)
}

func convertInt64IntoBytes(num int64) []byte {
buf := make([]byte, binary.MaxVarintLen64)
n := binary.PutVarint(buf, num)
return buf[:n]
}
Loading