Adding Request/AvailableDAGs APIs #68

plauche · 2023-06-05T18:34:53Z

Adds the RequestAvailableDags API, and association between filenames and DAGs.

John-LittleBearLabs · 2023-06-06T16:37:26Z

local-storage/src/block.rs

@@ -30,6 +31,7 @@ impl fmt::Debug for StoredBlock {

        f.debug_struct("StoredBlock")
            .field("cid", &cid_str)
+            .field("filename", &self.filename)


It looks like you're storing the blocks in sqlite not in individual files (and if you were I would hope you could derive the filename from the CID).

So I'm guessing this filename member is actually from a UnixFS directory's link to this block? e.g. the directory entry's name for it?

The filename originates in this API message ApplicationAPI::ImportFile { path: String }. We just pull out the "file name" piece of the path in Storage::import_path.

John-LittleBearLabs · 2023-06-06T16:39:57Z

local-storage/src/storage.rs

@@ -68,7 +69,11 @@ impl Storage {
            }
        }
        if let Some(root_cid) = root_cid {
+            if let Some(filename) = path.file_name().and_then(|p| p.to_str()) {
+                self.provider.name_dag(&root_cid, filename)?;


Oh, interesting - you're naming root CIDs. I think Pinata does this IIRC (but doesn't really expose it via gateway).

John-LittleBearLabs · 2023-06-06T17:02:18Z

local-storage/src/provider.rs

+        window_size: Option<u32>,
+    ) -> Result<Vec<StoredBlock>> {
+        let mut base_query = "
+        WITH RECURSIVE cids(x,y,z) AS (


I was not familiar with WITH RECURSIVE. That's interesting.
How is performance? Is this function (regardless of how it's implemented) a memory limitation concern?

I haven't run into any show stopping performance issues with files in the 5-50 mB range, but also haven't done much memory profiling around larger files here. I think we are Ok for the smaller files scheduled for usage in the demonstration.

John-LittleBearLabs · 2023-06-06T17:09:03Z

local-storage/src/provider.rs

 }

 impl StorageProvider for SqliteStorageProvider {
    fn import_block(&self, block: &StoredBlock) -> Result<()> {
        self.conn.execute(
-            "INSERT OR IGNORE INTO blocks (cid, data) VALUES (?1, ?2)",
-            (&block.cid, &block.data),
+            "INSERT OR IGNORE INTO blocks (cid, data, filename) VALUES (?1, ?2, ?3)",


The "or ignore" makes me immediately question what the PK and/or unique constraints are. And looking at that code leads me to a side question: The purpose of an autonumber-style "id"?

But back to the main question: it looks like you did not modify the unique index on CID. So if you add a second file with the same content it gets silently dropped completely - no record of that filename existing. Is this really desired?

Perhaps it's too academic a concern, but in a pure mathematical sense a filename probably should't be a column in blocks, right? It would be a separate table that associates... I guess many-to-many?

John-LittleBearLabs · 2023-06-06T17:11:21Z

local-storage/src/provider.rs

            })?
            // TODO: Correctly catch/log/handle errors here
            .filter_map(|cid| cid.ok())
            .collect();
        Ok(roots)
    }

+    fn name_dag(&self, cid: &str, file_name: &str) -> Result<()> {


Conversely this method assumes you want to drop the old name.

I would've guessed there'd be a parameter, perhaps bool or bool-equivalent-enum, specifying whether you'd like to do this or do WHERE cid = ?2 AND filename = NULL

John-LittleBearLabs · 2023-06-06T17:16:58Z

local-storage/src/provider.rs

+        )?;
+        Ok(())
+    }
+
    fn get_missing_cid_blocks(&self, cid: &str) -> Result<Vec<String>> {
        // First get all block cid+id associated with root cid
        let blocks: Vec<(String, Option<i32>)> = self


Curious that you don't want to use a recursive select here?

I went ahead and fixed this for consistency and in case it becomes an issue later

John-LittleBearLabs · 2023-06-06T17:21:38Z

messages/src/api.rs

@@ -103,6 +116,11 @@ pub enum ApplicationAPI {
    Version {
        version: String,
    },
+    RequestAvailableDags,


Although I assume it wouldn't be a concern here, are there any cross-version compatibility concerns with changes to this enum? Like, is it append-only for new enum variants or whatever?

Currently there is no assumed cross-version compatibility for these enums. For the purposes of the initial demonstrations we are assuming that all sides are running the same version, and that we have the control to make that so.

A future enhancement along the path to SDK-ifying this software would be building more cross-version compatibility into these enums.

John-LittleBearLabs · 2023-06-06T17:23:12Z

myceli/src/listener.rs

@@ -213,6 +213,9 @@ impl<T: Transport + Send + 'static> Listener<T> {
                }
                None
            }
+            Message::ApplicationAPI(ApplicationAPI::RequestAvailableDags) => {
+                Some(handlers::get_available_dags(self.storage.clone())?)


Unfortunate that this sort of mapping can't be generated.

John-LittleBearLabs · 2023-06-06T17:24:52Z

myceli/src/shipper.rs

@@ -418,6 +418,7 @@ impl<T: Transport + Send + 'static> Shipper<T> {
            cid: Cid::try_from(block.cid)?.to_string(),
            data: block.data,
            links,
+            filename: block.filename,


I may very well be misunderstanding the purpose of this mod, but are the various satellites & ground stations supposed to agree on the filenames of roots?

The idea is to give recognizable names to DAGs/root CIDs for recognition by operators and/or observers. Maintaining the same "file name" across satellites & ground stations will hopefully help folks recognize the completion of a DAG transfer.

John-LittleBearLabs

I do think at some point we should pick one way handle conflicting names (old wins silently, new wins silently, error) and use the same approach in both import_dag and also name_dag.

But if, for right now, the answer is "it doesn't matter which happens", then this is fine. We could, for example, simply ask that they never use a file with the same content as another file they've used.

plauche · 2023-06-07T15:03:53Z

I do think at some point we should pick one way handle conflicting names (old wins silently, new wins silently, error) and use the same approach in both import_dag and also name_dag.

But if, for right now, the answer is "it doesn't matter which happens", then this is fine. We could, for example, simply ask that they never use a file with the same content as another file they've used.

Sorry, I didn't mean to ignore your two comments on this! Yes I agree that it should be addressed at some point. I created #69 to address this in the future.

plauche added 5 commits June 6, 2023 10:37

Implementing Get/AvailableDags API, including filename for DAGs

08432a6

Simplifed dag fetching and fixed single-block dags

c495a0e

Transmitting dags with filenames

47bf878

Cleaning up some storage code

7ae8a65

Changelog bump

299bbb0

plauche force-pushed the feat/request-available-dags branch from 8f4485c to 299bbb0 Compare June 6, 2023 15:39

plauche changed the title ~~Adding Get/AvailableDAGs APIs~~ Adding Request/AvailableDAGs APIs Jun 6, 2023

Removing prints

1554345

plauche marked this pull request as ready for review June 6, 2023 15:44

John-LittleBearLabs reviewed Jun 6, 2023

View reviewed changes

Make get_missing_cid_blocks recursive

f2bf456

plauche requested a review from John-LittleBearLabs June 7, 2023 14:20

John-LittleBearLabs approved these changes Jun 7, 2023

View reviewed changes

plauche mentioned this pull request Jun 7, 2023

Handle with conflicting file names for DAGs #69

Open

plauche merged commit 07e56f4 into main Jun 7, 2023

plauche deleted the feat/request-available-dags branch June 7, 2023 15:04

plauche mentioned this pull request Jun 9, 2023

[DEMO] IPFS Thing 2023 Prep #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Request/AvailableDAGs APIs #68

Adding Request/AvailableDAGs APIs #68

plauche commented Jun 5, 2023 •

edited

Loading

John-LittleBearLabs Jun 6, 2023

plauche Jun 7, 2023

John-LittleBearLabs Jun 6, 2023

John-LittleBearLabs Jun 6, 2023

plauche Jun 7, 2023

John-LittleBearLabs Jun 6, 2023

John-LittleBearLabs Jun 6, 2023

John-LittleBearLabs Jun 6, 2023

plauche Jun 7, 2023

John-LittleBearLabs Jun 6, 2023

plauche Jun 7, 2023

plauche Jun 7, 2023

John-LittleBearLabs Jun 6, 2023

John-LittleBearLabs Jun 6, 2023

plauche Jun 7, 2023 •

edited

Loading

John-LittleBearLabs left a comment

plauche commented Jun 7, 2023

Adding Request/AvailableDAGs APIs #68

Adding Request/AvailableDAGs APIs #68

Conversation

plauche commented Jun 5, 2023 • 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

plauche Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

John-LittleBearLabs left a comment

Choose a reason for hiding this comment

plauche commented Jun 7, 2023

plauche commented Jun 5, 2023 •

edited

Loading

plauche Jun 7, 2023 •

edited

Loading