GO-3946 | GO-3587 | GO-4130 - Gallery cache #1531

KirillSto · 2024-09-02T09:44:04Z

https://linear.app/anytype/issue/GO-3946/add-cache-for-index-and-for-usecase
https://linear.app/anytype/issue/GO-3587/add-cache-on-gallerydownloadindex
https://linear.app/anytype/issue/GO-4130/substitute-current-get-started-usecase-with-new-one

Introduce gallery.Service, where all logic with import of gallery usecases is accumulated
Introduce IndexCache that saves latest index locally
Introduce DashboardInjector that injects Homepage object during ooooold space migration
Import built-in usecases using experience pipeline (it is possible due to widget and workspace being added to archives)
Add cache parameter to all gallery-related RPCs
Experience import uses clientCache in case built-in archive could not be downloaded from remote
Renewed Get Started usecase (Welcome letter)
Slight changes on Usecase validator

github-actions · 2024-09-24T12:30:38Z

New Coverage	48.6% of statements
Patch Coverage	61.5% of changed statements (273/444)

^{Coverage provided by https://github.com/seriousben/go-patch-cover-action}

AnastasiaShemyakinskaya · 2024-09-24T13:57:14Z

core/block/import/pb/converter.go

@@ -163,7 +163,7 @@ func (p *Pb) handleImportPath(
 		needToImportWidgets = p.needToImportWidgets(profile.Address, pr.AccountId)
 		profileID = profile.ProfileId
 	}
-	return p.getSnapshotsFromProvidedFiles(pathCount, importSource, allErrors, path, profileID, needToImportWidgets, isMigration, importType)
+	return p.getSnapshotsFromProvidedFiles(pathCount, importSource, allErrors, path, profileID, needToImportWidgets, isMigration, isNewSpace, importType)


The signature is very huge now, may be we can introduce some dataObject or request object with all following parameters?

I have refactored whole converter.go a little bit. Actaully, we can save a lot of info inside Pb struct state, so there is no need to transfer it throghout method calls. Please check if I could miss smth

AnastasiaShemyakinskaya · 2024-09-24T14:00:16Z

core/gallery/cache.go

+}
+
+func (c *cache) GetManifest(downloadLink string, timeoutInSeconds int) (info *model.ManifestInfo, err error) {
+	localIndex, err := c.storage.getIndex()


May be extract some duplicate code from GetIndex and GetManifest? Looks like they have a lot of common code?

Yes, it hepled to remove 30 lines 😄

AnastasiaShemyakinskaya · 2024-09-24T14:01:30Z

core/gallery/cache.go

+	}
+
+	if errors.Is(err, ErrNotModified) {
+		manifest, err := getManifestByDownloadLink(localIndex, downloadLink)


Can be just return getManifestByDownloadLink(localIndex, downloadLink)?

AnastasiaShemyakinskaya · 2024-09-24T14:01:37Z

core/gallery/cache.go

+
+	if localIndex != nil {
+		log.Warn("failed to download index from remote. Returning local index", zap.Error(err))
+		manifest, err := getManifestByDownloadLink(localIndex, downloadLink)


AnastasiaShemyakinskaya · 2024-09-24T14:05:58Z

core/gallery/cache.go

+		}
+	}
+
+	info.Description = stripTags(info.Description)


I think, it should be moved out of function, because it doesn't relate to validation, because according to name, this function is supposed to just validate manifest?🧐

I have changed strip logic a lil bit. Now it accepts manifest and is not sticked to validation

AnastasiaShemyakinskaya · 2024-09-24T14:18:55Z

util/dashboardinjector/injector.go

+		return err
+	}
+
+	if rmErr := os.Remove(path); rmErr != nil {


Should this logic also be executed in case of error?

Yes, it should. In case we have run into some problem on import stage, we still need to remove temp file

Then it should be in defer?

Yeah, thank you! I put it into defer and also fixed error return. On import failure I used to return wrong error 🙈

AnastasiaShemyakinskaya · 2024-09-24T14:20:54Z

core/gallery/service.go

+	}
+
+	if cachePath == "" {
+		if ctx == nil {


May be if ImportBuiltInUseCase should be called with context.Background() instead of nil to avoid such check?

Yeah, I have added ctx to all calls, thank you!

AnastasiaShemyakinskaya · 2024-09-24T14:23:13Z

core/gallery/service.go

+
+func getArchiveReaderAndSize(url string) (reader io.ReadCloser, size int64, err error) {
+	client := http.Client{Timeout: 15 * time.Second}
+	// nolint: gosec


Why do we need this comment? It seems like there is no security issues? 🧐

Actally there is. Linter worried about referencing some mal resources 🙈

AnastasiaShemyakinskaya · 2024-09-24T14:23:31Z

core/gallery/service.go

+}
+
+func generateMapOfDownloadLinksByNames(index *pb.RpcGalleryDownloadIndexResponse) map[string]string {
+	m := make(map[string]string)


Preallocate

AnastasiaShemyakinskaya · 2024-09-24T14:24:48Z

core/gallery/service.go

+	}
+	defer r.Close()
+
+	archives = make(map[string][]byte)


Allocate it near usage please

KirillSto added 2 commits August 29, 2024 11:06

GO-3946 Gallery cache WIP

b3e0cd5

GO-3946 Gallery svc and cache WIP

153493f

KirillSto self-assigned this Sep 2, 2024

KirillSto added 6 commits September 5, 2024 10:39

GO-3946 Work with client cache WIP

a792662

GO-3946 Remove archives

89e3e94

GO-3946 Add builtins back

32fafce

GO-3946 Merge main

8d45fbc

GO-3946 Write tests WIP

d7f01cd

GO-3946 Tests on ImportExperience

6e5e0d2

KirillSto marked this pull request as ready for review September 24, 2024 11:29

GO-3946 Fix lint and add mocks

79c3e98

KirillSto requested review from AnastasiaShemyakinskaya and requilence September 24, 2024 11:50

GO-3946 Fix lint and integration test

40ed419

KirillSto changed the title ~~GO-3946 Gallery cache~~ GO-3946 | GO-3587 - Gallery cache Sep 24, 2024

GO-3946 Check uint64 value on conversion

4d5ab1f

AnastasiaShemyakinskaya reviewed Sep 24, 2024

View reviewed changes

KirillSto added 4 commits September 24, 2024 16:39

GO-3946 Fix test

4d403ad

GO-3946 Add nolint

c730000

GO-3946 Save state of Pb on snapshot getting

b72daf9

GO-3946 Fix comments

26dd64b

KirillSto requested a review from AnastasiaShemyakinskaya September 25, 2024 23:25

KirillSto added 4 commits September 26, 2024 12:19

GO-3946 Merge main

8e297dc

GO-3946 Fix comments

cec39e9

GO-3946 Merge main 2

899e58c

GO-3946 Fix comment

60c731c

AnastasiaShemyakinskaya approved these changes Sep 30, 2024

View reviewed changes

GO-4130 Update Get Started

43546a9

KirillSto changed the title ~~GO-3946 | GO-3587 - Gallery cache~~ GO-3946 | GO-3587 | GO-4130 - Gallery cache Oct 2, 2024

GO-3946 Merge main 3

c773370

requilence mentioned this pull request Oct 16, 2024

GO-4130 substitute current get started usecase with new one #1696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GO-3946 | GO-3587 | GO-4130 - Gallery cache #1531

GO-3946 | GO-3587 | GO-4130 - Gallery cache #1531

KirillSto commented Sep 2, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024 •

edited

Loading

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 24, 2024

AnastasiaShemyakinskaya Sep 24, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 26, 2024

KirillSto Sep 27, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

AnastasiaShemyakinskaya Sep 24, 2024

KirillSto Sep 25, 2024

GO-3946 | GO-3587 | GO-4130 - Gallery cache #1531

Are you sure you want to change the base?

GO-3946 | GO-3587 | GO-4130 - Gallery cache #1531

Conversation

KirillSto commented Sep 2, 2024 • edited Loading

github-actions bot commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillSto commented Sep 2, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024 •

edited

Loading