From 12c28e537de2669cab2f2bc33ebed13fd7f4be46 Mon Sep 17 00:00:00 2001 From: Jarek Kowalski Date: Wed, 9 May 2018 22:19:36 -0700 Subject: [PATCH] simplified ObjectID code and changed it to a string --- cli/objref.go | 12 +- .../repository/upload_download_objects.go | 2 +- fs/cachefs/cache.go | 2 +- object/object_manager.go | 43 +++--- object/object_manager_test.go | 22 ++- object/object_writer.go | 14 +- object/objectid.go | 141 ++++++++---------- object/objectid_test.go | 77 ++++------ repo/repository_test.go | 22 +-- snapshot/upload.go | 18 +-- 10 files changed, 163 insertions(+), 190 deletions(-) diff --git a/cli/objref.go b/cli/objref.go index 3cfdd177e..ed8203430 100644 --- a/cli/objref.go +++ b/cli/objref.go @@ -14,12 +14,12 @@ import ( func parseObjectID(ctx context.Context, mgr *snapshot.Manager, id string) (object.ID, error) { head, tail := splitHeadTail(id) if len(head) == 0 { - return object.NullID, fmt.Errorf("invalid object ID: %v", id) + return "", fmt.Errorf("invalid object ID: %v", id) } oid, err := object.ParseID(head) if err != nil { - return object.NullID, fmt.Errorf("can't parse object ID %v: %v", head, err) + return "", fmt.Errorf("can't parse object ID %v: %v", head, err) } if tail == "" { @@ -28,7 +28,7 @@ func parseObjectID(ctx context.Context, mgr *snapshot.Manager, id string) (objec dir := mgr.DirectoryEntry(oid) if err != nil { - return object.NullID, err + return "", err } return parseNestedObjectID(ctx, dir, tail) @@ -41,17 +41,17 @@ func parseNestedObjectID(ctx context.Context, startingDir fs.Directory, id strin for head != "" { dir, ok := current.(fs.Directory) if !ok { - return object.NullID, fmt.Errorf("entry not found '%v': parent is not a directory", head) + return "", fmt.Errorf("entry not found '%v': parent is not a directory", head) } entries, err := dir.Readdir(ctx) if err != nil { - return object.NullID, err + return "", err } e := entries.FindByName(head) if e == nil { - return object.NullID, fmt.Errorf("entry not found: '%v'", head) + return "", fmt.Errorf("entry not found: '%v'", head) } current = e diff --git a/examples/repository/upload_download_objects.go b/examples/repository/upload_download_objects.go index eca61a0ee..66ccd083d 100644 --- a/examples/repository/upload_download_objects.go +++ b/examples/repository/upload_download_objects.go @@ -23,7 +23,7 @@ func uploadRandomObject(ctx context.Context, r *repo.Repository, length int) (ob } rand.Read(buf[0:todo]) //nolint:errcheck if _, err := w.Write(buf[0:todo]); err != nil { - return object.NullID, err + return "", err } length -= todo } diff --git a/fs/cachefs/cache.go b/fs/cachefs/cache.go index b9984b91f..279a48e21 100644 --- a/fs/cachefs/cache.go +++ b/fs/cachefs/cache.go @@ -80,7 +80,7 @@ type Loader func(ctx context.Context) (fs.Entries, error) // the results. func (c *Cache) Readdir(ctx context.Context, d fs.Directory) (fs.Entries, error) { if h, ok := d.(object.HasObjectID); ok { - cacheID := h.ObjectID().String() + cacheID := string(h.ObjectID()) cacheExpiration := 24 * time.Hour return c.getEntries(ctx, cacheID, cacheExpiration, d.Readdir) } diff --git a/object/object_manager.go b/object/object_manager.go index d30bfb958..2825d066c 100644 --- a/object/object_manager.go +++ b/object/object_manager.go @@ -74,8 +74,8 @@ func (om *Manager) Open(ctx context.Context, objectID ID) (Reader, error) { // Flush any pending writes. om.writeBackWG.Wait() - if objectID.Indirect != nil { - rd, err := om.Open(ctx, *objectID.Indirect) + if indexObjectID, ok := objectID.IndexObjectID(); ok { + rd, err := om.Open(ctx, indexObjectID) if err != nil { return nil, err } @@ -114,11 +114,11 @@ func (om *Manager) VerifyObject(ctx context.Context, oid ID) (int64, []string, e return l, blocks.blockIDs(), nil } -func (om *Manager) verifyIndirectObjectInternal(ctx context.Context, oid ID, blocks *blockTracker) (int64, error) { - if _, err := om.verifyObjectInternal(ctx, *oid.Indirect, blocks); err != nil { +func (om *Manager) verifyIndirectObjectInternal(ctx context.Context, indexObjectID ID, blocks *blockTracker) (int64, error) { + if _, err := om.verifyObjectInternal(ctx, indexObjectID, blocks); err != nil { return 0, fmt.Errorf("unable to read index: %v", err) } - rd, err := om.Open(ctx, *oid.Indirect) + rd, err := om.Open(ctx, indexObjectID) if err != nil { return 0, err } @@ -136,7 +136,7 @@ func (om *Manager) verifyIndirectObjectInternal(ctx context.Context, oid ID, blo } if l != m.Length { - return 0, fmt.Errorf("unexpected length of part %#v of indirect object %q: %v %v, expected %v", i, oid, m.Object, l, m.Length) + return 0, fmt.Errorf("unexpected length of part %#v of indirect object %q: %v %v, expected %v", i, indexObjectID, m.Object, l, m.Length) } } @@ -145,16 +145,21 @@ func (om *Manager) verifyIndirectObjectInternal(ctx context.Context, oid ID, blo } func (om *Manager) verifyObjectInternal(ctx context.Context, oid ID, blocks *blockTracker) (int64, error) { - if oid.Indirect != nil { - return om.verifyIndirectObjectInternal(ctx, oid, blocks) + if indexObjectID, ok := oid.IndexObjectID(); ok { + return om.verifyIndirectObjectInternal(ctx, indexObjectID, blocks) } - p, err := om.blockMgr.BlockInfo(ctx, oid.ContentBlockID) - if err != nil { - return 0, err + if blockID, ok := oid.BlockID(); ok { + p, err := om.blockMgr.BlockInfo(ctx, blockID) + if err != nil { + return 0, err + } + blocks.addBlock(blockID) + return int64(p.Length), nil } - blocks.addBlock(oid.ContentBlockID) - return int64(p.Length), nil + + return 0, fmt.Errorf("unrecognized object type: %v", oid) + } // Flush closes any pending pack files. Once this method returns, ObjectIDs returned by ObjectManager are @@ -249,12 +254,16 @@ func (om *Manager) flattenListChunk(rawReader io.Reader) ([]indirectObjectEntry, } func (om *Manager) newRawReader(ctx context.Context, objectID ID) (Reader, error) { - payload, err := om.blockMgr.GetBlock(ctx, objectID.ContentBlockID) - if err != nil { - return nil, err + if blockID, ok := objectID.BlockID(); ok { + payload, err := om.blockMgr.GetBlock(ctx, blockID) + if err != nil { + return nil, err + } + + return newObjectReaderWithData(payload), nil } - return newObjectReaderWithData(payload), nil + return nil, fmt.Errorf("unsupported object ID: %v", objectID) } type readerWithData struct { diff --git a/object/object_manager_test.go b/object/object_manager_test.go index 6337e9a70..002d63ce9 100644 --- a/object/object_manager_test.go +++ b/object/object_manager_test.go @@ -92,9 +92,9 @@ func TestWriters(t *testing.T) { }{ { []byte("the quick brown fox jumps over the lazy dog"), - ID{ContentBlockID: "77add1d5f41223d5582fca736a5cb335"}, + "D77add1d5f41223d5582fca736a5cb335", }, - {make([]byte, 100), ID{ContentBlockID: "6d0bb00954ceb7fbee436bb55a8397a9"}}, // 100 zero bytes + {make([]byte, 100), "D6d0bb00954ceb7fbee436bb55a8397a9"}, // 100 zero bytes } for _, c := range cases { @@ -116,7 +116,7 @@ func TestWriters(t *testing.T) { t.Errorf("incorrect result for %v, expected: %v got: %v", c.data, c.objectID.String(), result.String()) } - if c.objectID.ContentBlockID == "" { + if _, ok := c.objectID.BlockID(); !ok { if len(data) != 0 { t.Errorf("unexpected data written to the storage: %v", data) } @@ -142,17 +142,14 @@ func TestWriterCompleteChunkInTwoWrites(t *testing.T) { writer.Write(bytes[0:50]) writer.Write(bytes[0:50]) result, err := writer.Result() - if !objectIDsEqual(result, ID{ContentBlockID: "6d0bb00954ceb7fbee436bb55a8397a9"}) { + if !objectIDsEqual(result, "D6d0bb00954ceb7fbee436bb55a8397a9") { t.Errorf("unexpected result: %v err: %v", result, err) } } func verifyIndirectBlock(ctx context.Context, t *testing.T, r *Manager, oid ID) { - for oid.Indirect != nil { - direct := *oid.Indirect - oid = direct - - rd, err := r.Open(ctx, direct) + for indexBlockID, isIndirect := oid.IndexObjectID(); isIndirect; indexBlockID, isIndirect = indexBlockID.IndexObjectID() { + rd, err := r.Open(ctx, indexBlockID) if err != nil { t.Errorf("unable to open %v: %v", oid.String(), err) return @@ -233,11 +230,12 @@ func TestIndirection(t *testing.T) { } func indirectionLevel(oid ID) int { - if oid.Indirect == nil { + indexObjectID, ok := oid.IndexObjectID() + if !ok { return 0 } - return 1 + indirectionLevel(*oid.Indirect) + return 1 + indirectionLevel(indexObjectID) } func TestHMAC(t *testing.T) { @@ -297,7 +295,7 @@ func TestReaderStoredBlockNotFound(t *testing.T) { ctx := context.Background() _, om := setupTest(t) - objectID, err := ParseID("Dno-such-block") + objectID, err := ParseID("Ddeadbeef") if err != nil { t.Errorf("cannot parse object ID: %v", err) } diff --git a/object/object_writer.go b/object/object_writer.go index ed4ef6a84..96b938f02 100644 --- a/object/object_writer.go +++ b/object/object_writer.go @@ -104,7 +104,7 @@ func (w *objectWriter) flushBuffer() error { return } - w.blockIndex[chunkID].Object = ID{ContentBlockID: blockID} + w.blockIndex[chunkID].Object = DirectObjectID(blockID) } // When writing pack internal object don't use asynchronous write, since we're already under the semaphore @@ -131,13 +131,13 @@ func (w *objectWriter) flushBuffer() error { func (w *objectWriter) Result() (ID, error) { if w.buffer.Len() > 0 || len(w.blockIndex) == 0 { if err := w.flushBuffer(); err != nil { - return NullID, err + return "", err } } w.pendingBlocksWG.Wait() if err := w.err.check(); err != nil { - return NullID, err + return "", err } if len(w.blockIndex) == 1 { @@ -154,17 +154,17 @@ func (w *objectWriter) Result() (ID, error) { jw := jsonstream.NewWriter(iw, indirectStreamType) for _, e := range w.blockIndex { if err := jw.Write(&e); err != nil { - return NullID, fmt.Errorf("unable to write indirect block index: %v", err) + return "", fmt.Errorf("unable to write indirect block index: %v", err) } } if err := jw.Finalize(); err != nil { - return NullID, fmt.Errorf("unable to finalize indirect block index: %v", err) + return "", fmt.Errorf("unable to finalize indirect block index: %v", err) } oid, err := iw.Result() if err != nil { - return NullID, err + return "", err } - return ID{Indirect: &oid}, nil + return IndirectObjectID(oid), nil } // WriterOptions can be passed to Repository.NewWriter() diff --git a/object/objectid.go b/object/objectid.go index c8e5cedab..749307798 100644 --- a/object/objectid.go +++ b/object/objectid.go @@ -1,104 +1,91 @@ package object import ( - "encoding/json" - + "encoding/hex" "fmt" + "strings" ) -// ID is an identifier of a repository object. Repository objects can be stored: +// ID is an identifier of a repository object. Repository objects can be stored. // -// 1. In a single content block, this is the most common case for small objects. -// 2. In a series of content blocks with an indirect block pointing at them (multiple indirections are allowed). This is used for larger files. -// -type ID struct { - ContentBlockID string - Indirect *ID -} - -// MarshalJSON emits ObjectID in standard string format. -func (oid *ID) MarshalJSON() ([]byte, error) { - s := oid.String() - return json.Marshal(&s) -} - -// UnmarshalJSON unmarshals Object ID from a JSON string. -func (oid *ID) UnmarshalJSON(b []byte) error { - var s string - err := json.Unmarshal(b, &s) - if err != nil { - return err - } - - *oid, err = ParseID(s) - return err -} +// 1. In a single content block, this is the most common case for small objects. Such object IDs start with "D" +// 2. In a series of content blocks with an indirect block pointing at them (multiple indirections are allowed). +// This is used for larger files. Object IDs using indirect blocks start with "I" +type ID string // HasObjectID exposes the identifier of an object. type HasObjectID interface { ObjectID() ID } -// NullID is the identifier of an null/empty object. -var NullID ID - // String returns string representation of ObjectID that is suitable for displaying in the UI. -// -// Note that the object ID name often contains its encryption key, which is sensitive and can be quite long (~100 characters long). -func (oid ID) String() string { - if oid.Indirect != nil { - return fmt.Sprintf("I%v", oid.Indirect) - } - - if oid.ContentBlockID != "" { - return "D" + oid.ContentBlockID - } - - return "B" +func (i ID) String() string { + return string(i) } -// Validate validates the ObjectID structure. -func (oid *ID) Validate() error { - var c int - if len(oid.ContentBlockID) > 0 { - c++ +// IndexObjectID returns the object ID of the underlying index object. +func (i ID) IndexObjectID() (ID, bool) { + if strings.HasPrefix(string(i), "I") { + return i[1:], true } - if oid.Indirect != nil { - c++ - if err := oid.Indirect.Validate(); err != nil { - return fmt.Errorf("invalid indirect object ID %v: %v", oid, err) + return "", false +} + +// BlockID returns the block ID of the underlying content storage block. +func (i ID) BlockID() (string, bool) { + if strings.HasPrefix(string(i), "D") { + return string(i[1:]), true + } + + return "", false +} + +// Validate checks the ID format for validity and reports any errors. +func (i ID) Validate() error { + if indexObjectID, ok := i.IndexObjectID(); ok { + if err := indexObjectID.Validate(); err != nil { + return fmt.Errorf("invalid indirect object ID %v: %v", i, err) } + + return nil } - if c != 1 { - return fmt.Errorf("inconsistent block content: %+v", oid) - } + if blockID, ok := i.BlockID(); ok { + if len(blockID) < 2 { + return fmt.Errorf("missing block ID") + } - return nil -} - -// ParseID converts the specified string into ObjectID. -// The string format matches the output of String() method. -func ParseID(s string) (ID, error) { - if len(s) >= 1 { - chunkType := s[0] - content := s[1:] - - switch chunkType { - case 'I', 'D': - if chunkType == 'I' { - base, err := ParseID(content) - if err != nil { - return NullID, err - } - - return ID{Indirect: &base}, nil + // odd length - firstcharacter must be a single character between 'g' and 'z' + if len(blockID)%2 == 1 { + if blockID[0] < 'g' || blockID[0] > 'z' { + return fmt.Errorf("invalid block ID prefix: %v", blockID) } - - return ID{ContentBlockID: content}, nil + blockID = blockID[1:] } + + if _, err := hex.DecodeString(blockID); err != nil { + return fmt.Errorf("invalid blockID suffix, must be base-16 encoded: %v", blockID) + } + + return nil } - return NullID, fmt.Errorf("malformed object id: '%s'", s) + return fmt.Errorf("invalid object ID: %v", i) +} + +// DirectObjectID returns direct object ID based on the provided block ID. +func DirectObjectID(blockID string) ID { + return ID("D" + blockID) +} + +// IndirectObjectID returns indirect object ID based on the underlying index object ID. +func IndirectObjectID(indexObjectID ID) ID { + return "I" + indexObjectID +} + +// ParseID converts the specified string into object ID +func ParseID(s string) (ID, error) { + i := ID(s) + return i, i.Validate() } diff --git a/object/objectid_test.go b/object/objectid_test.go index 72d242d6f..1aa29484b 100644 --- a/object/objectid_test.go +++ b/object/objectid_test.go @@ -1,8 +1,6 @@ package object import ( - "reflect" - "strings" "testing" ) @@ -10,54 +8,41 @@ type rawObjectID ID func TestParseObjectID(t *testing.T) { cases := []struct { - Text string - ObjectID ID + text string + isValid bool }{ - {"Dfoo", ID{ContentBlockID: "foo"}}, - {"IDfoo", ID{Indirect: &ID{ContentBlockID: "foo"}}}, - {"IDfoo", ID{Indirect: &ID{ContentBlockID: "foo"}}}, - {"IIDfoo", ID{Indirect: &ID{Indirect: &ID{ContentBlockID: "foo"}}}}, + {"Df0f0", true}, + {"IDf0f0", true}, + {"IDf0f0", true}, + {"IIDf0f0", true}, + {"Dxf0f0", true}, + {"IDxf0f0", true}, + {"IDxf0f0", true}, + {"IIDxf0f0", true}, + {"Dxf0f", false}, + {"IDxf0f", false}, + {"Da", false}, + {"Daf0f0", false}, + {"", false}, + {"B!$@#$!@#$", false}, + {"X", false}, + {"I.", false}, + {"I.x", false}, + {"I.af", false}, + {"Ix.ag", false}, + {"Iab.", false}, + {"I1", false}, + {"I1,", false}, + {"I-1,X", false}, + {"Xsomething", false}, } for _, tc := range cases { - oid, err := ParseID(tc.Text) - if err != nil { - t.Errorf("error parsing %q: %v", tc.Text, err) - } - - if !reflect.DeepEqual(oid, tc.ObjectID) { - t.Errorf("invalid result for %q: %+v, wanted %+v", tc.Text, rawObjectID(oid), rawObjectID(tc.ObjectID)) - } - - oid2, err := ParseID(oid.String()) - if err != nil { - t.Errorf("parse error %q: %v", oid.String(), err) - } else if !reflect.DeepEqual(oid, oid2) { - t.Errorf("does not round-trip: %v (%+v), got %+v", oid.String(), rawObjectID(oid), rawObjectID(oid2)) - } - } -} - -func TestParseMalformedObjectID(t *testing.T) { - cases := []string{ - "", - "B!$@#$!@#$", - "X", - "I.", - "I.x", - "I.af", - "Ix.ag", - "Iab.", - "I1", - "I1,", - "I-1,X", - "Xsomething", - } - - for _, c := range cases { - v, err := ParseID(c) - if !reflect.DeepEqual(v, NullID) || err == nil || !strings.HasPrefix(err.Error(), "malformed object id") { - t.Errorf("unexpected result for %v: v: %v err: %v", c, v, err) + _, err := ParseID(tc.text) + if err != nil && tc.isValid { + t.Errorf("error parsing %q: %v", tc.text, err) + } else if err == nil && !tc.isValid { + t.Errorf("unexpected success parsing %v", tc.text) } } } diff --git a/repo/repository_test.go b/repo/repository_test.go index 3f148cb75..fa03431d3 100644 --- a/repo/repository_test.go +++ b/repo/repository_test.go @@ -63,9 +63,9 @@ func TestWriters(t *testing.T) { }{ { []byte("the quick brown fox jumps over the lazy dog"), - object.ID{ContentBlockID: "77add1d5f41223d5582fca736a5cb335"}, + "D77add1d5f41223d5582fca736a5cb335", }, - {make([]byte, 100), object.ID{ContentBlockID: "6d0bb00954ceb7fbee436bb55a8397a9"}}, // 100 zero bytes + {make([]byte, 100), "D6d0bb00954ceb7fbee436bb55a8397a9"}, // 100 zero bytes } ctx := context.Background() @@ -128,7 +128,7 @@ func TestWriterCompleteChunkInTwoWrites(t *testing.T) { writer.Write(bytes[0:50]) writer.Write(bytes[0:50]) result, err := writer.Result() - if !objectIDsEqual(result, object.ID{ContentBlockID: "6d0bb00954ceb7fbee436bb55a8397a9"}) { + if result != "D6d0bb00954ceb7fbee436bb55a8397a9" { t.Errorf("unexpected result: %v err: %v", result, err) } } @@ -249,7 +249,7 @@ func TestReaderStoredBlockNotFound(t *testing.T) { _, _, repo := setupTest(t) ctx := context.Background() - objectID, err := object.ParseID("Dno-such-block") + objectID, err := object.ParseID("Ddeadbeef") if err != nil { t.Errorf("cannot parse object ID: %v", err) } @@ -349,26 +349,20 @@ func TestFormats(t *testing.T) { n.noHMAC = true }, oids: map[string]object.ID{ - "": object.ID{ContentBlockID: "d41d8cd98f00b204e9800998ecf8427e"}, - "The quick brown fox jumps over the lazy dog": object.ID{ - ContentBlockID: "9e107d9d372bb6826bd81d3542a419d6", - }, + "": "Dd41d8cd98f00b204e9800998ecf8427e", + "The quick brown fox jumps over the lazy dog": "D9e107d9d372bb6826bd81d3542a419d6", }, }, { format: makeFormat("UNENCRYPTED_HMAC_SHA256"), oids: map[string]object.ID{ - "The quick brown fox jumps over the lazy dog": object.ID{ - ContentBlockID: "f7bc83f430538424b13298e6aa6fb143ef4d59a14946175997479dbc2d1a3cd8", - }, + "The quick brown fox jumps over the lazy dog": "Df7bc83f430538424b13298e6aa6fb143ef4d59a14946175997479dbc2d1a3cd8", }, }, { format: makeFormat("UNENCRYPTED_HMAC_SHA256_128"), oids: map[string]object.ID{ - "The quick brown fox jumps over the lazy dog": object.ID{ - ContentBlockID: "f7bc83f430538424b13298e6aa6fb143", - }, + "The quick brown fox jumps over the lazy dog": "Df7bc83f430538424b13298e6aa6fb143", }, }, } diff --git a/snapshot/upload.go b/snapshot/upload.go index 2164779fb..5b479439a 100644 --- a/snapshot/upload.go +++ b/snapshot/upload.go @@ -226,7 +226,7 @@ func newDirEntry(md *fs.EntryMetadata, oid object.ID) *dir.Entry { func (u *Uploader) uploadFile(ctx context.Context, file fs.File) (object.ID, error) { res := u.uploadFileInternal(ctx, file, file.Metadata().Name) if res.err != nil { - return object.NullID, res.err + return "", res.err } return res.de.ObjectID, nil } @@ -243,19 +243,19 @@ func (u *Uploader) uploadDir(ctx context.Context, dir fs.Directory) (object.ID, oid, _, err := uploadDirInternal(ctx, u, dir, ".") if u.IsCancelled() { if err2 := u.cacheReader.CopyTo(u.cacheWriter); err != nil { - return object.NullID, object.NullID, err2 + return "", "", err2 } } defer u.cacheWriter.Finalize() //nolint:errcheck u.cacheWriter = nil if err != nil { - return object.NullID, object.NullID, err + return "", "", err } hcid, err := mw.Result() if flushErr := u.repo.Objects.Flush(ctx); flushErr != nil { - return object.NullID, object.NullID, fmt.Errorf("can't flush pending objects: %v", flushErr) + return "", "", fmt.Errorf("can't flush pending objects: %v", flushErr) } return oid, hcid, err } @@ -519,7 +519,7 @@ func uploadDirInternal( entries, direrr := directory.Readdir(ctx) if direrr != nil { - return object.NullID, dir.Summary{}, direrr + return "", dir.Summary{}, direrr } writer := u.repo.Objects.NewWriter(ctx, object.WriterOptions{ @@ -530,19 +530,19 @@ func uploadDirInternal( defer writer.Close() //nolint:errcheck if err := u.processSubdirectories(ctx, dirRelativePath, entries, dw, &summ); err != nil { - return object.NullID, dir.Summary{}, err + return "", dir.Summary{}, err } u.prepareProgress(dirRelativePath, entries) workItems, workItemErr := u.prepareWorkItems(ctx, dirRelativePath, entries, &summ) if workItemErr != nil { - return object.NullID, dir.Summary{}, workItemErr + return "", dir.Summary{}, workItemErr } if err := u.processUploadWorkItems(workItems, dw); err != nil { - return object.NullID, dir.Summary{}, err + return "", dir.Summary{}, err } if err := dw.Finalize(&summ); err != nil { - return object.NullID, dir.Summary{}, fmt.Errorf("unable to finalize directory: %v", err) + return "", dir.Summary{}, fmt.Errorf("unable to finalize directory: %v", err) } oid, err := writer.Result()