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

Iterating a specific kvsEnvStorage returns all key-value pairs for all databases #20

Closed
berkbuzcu opened this issue Feb 16, 2022 · 5 comments · Fixed by #21
Closed

Iterating a specific kvsEnvStorage returns all key-value pairs for all databases #20

berkbuzcu opened this issue Feb 16, 2022 · 5 comments · Fixed by #21
Labels
Type: Bug Bug or Bug fixes

Comments

@berkbuzcu
Copy link

Let's say I've got two collections:

const A = async () => {
return await kvsEnvStorage({
name: "a_col",
version: 1,
})
}

const B = async () => {
return await kvsEnvStorage({
name: "b_col",
version: 1,
})
}

const storage = await A()
for await (const [key, value] of storage) {
console.log(key, value)
}

Will print items that belong to storage B as well.

Is this an error of my usage, or a bug?

@azu
Copy link
Owner

azu commented Feb 16, 2022

Thanks to report!

Which env do you run this code?
@kvs/env work on browser and node

browser(IndexedDB sperate table name)

const iterator = <Schema extends KVSIndexedSchema, K extends StoreNames<Schema>, V extends StoreValue<Schema, K>>(
database: IDBDatabase,
tableName: string
): AsyncIterator<[K, V]> => {
const handleCursor = <T>(request: IDBRequest<T | null>): Promise<{ done: boolean; value?: T }> => {
return new Promise((resolve, reject) => {
request.onsuccess = () => {
const cursor = request.result;
if (!cursor) {
return resolve({
done: true
});
}
return resolve({
done: false,
value: cursor
});
};
request.onerror = () => {
reject(request.error);
};
});
};
const transaction = database.transaction(tableName, "readonly");
const objectStore = transaction.objectStore(tableName);
const request = objectStore.openCursor();
return {
async next() {
const { done, value } = await handleCursor(request);
if (!done) {
const storageKey = value?.key as K;
const storageValue = value?.value as V;
value?.continue();
return { done: false, value: [storageKey, storageValue] };
}
return { done: true, value: undefined };
}
};
};

node

const { name, version, upgrade, ...kvStorageOptions } = options;
const kvsVersionKey = kvStorageOptions.kvsVersionKey ?? "__kvs_version__";
const storage = await openStorage({
storage: options.storage,
version: options.version,
onUpgrade: ({ oldVersion, newVersion, storage }) => {
if (!options.upgrade) {
return;
}
return options.upgrade({
kvs: createStore({ storage, kvsVersionKey }),
oldVersion,
newVersion
});
},
kvsVersionKey
});
return createStore({ storage, kvsVersionKey });

It seems that it has a bug that does not separate namespace…
(name is not used…)

@azu azu added the Type: Bug Bug or Bug fixes label Feb 16, 2022
@berkbuzcu
Copy link
Author

Thanks to report!

Which env do you run this code? @kvs/env work on browser and node

browser(IndexedDB sperate table name)

const iterator = <Schema extends KVSIndexedSchema, K extends StoreNames<Schema>, V extends StoreValue<Schema, K>>(
database: IDBDatabase,
tableName: string
): AsyncIterator<[K, V]> => {
const handleCursor = <T>(request: IDBRequest<T | null>): Promise<{ done: boolean; value?: T }> => {
return new Promise((resolve, reject) => {
request.onsuccess = () => {
const cursor = request.result;
if (!cursor) {
return resolve({
done: true
});
}
return resolve({
done: false,
value: cursor
});
};
request.onerror = () => {
reject(request.error);
};
});
};
const transaction = database.transaction(tableName, "readonly");
const objectStore = transaction.objectStore(tableName);
const request = objectStore.openCursor();
return {
async next() {
const { done, value } = await handleCursor(request);
if (!done) {
const storageKey = value?.key as K;
const storageValue = value?.value as V;
value?.continue();
return { done: false, value: [storageKey, storageValue] };
}
return { done: true, value: undefined };
}
};
};

node

const { name, version, upgrade, ...kvStorageOptions } = options;
const kvsVersionKey = kvStorageOptions.kvsVersionKey ?? "__kvs_version__";
const storage = await openStorage({
storage: options.storage,
version: options.version,
onUpgrade: ({ oldVersion, newVersion, storage }) => {
if (!options.upgrade) {
return;
}
return options.upgrade({
kvs: createStore({ storage, kvsVersionKey }),
oldVersion,
newVersion
});
},
kvsVersionKey
});
return createStore({ storage, kvsVersionKey });

It seems that it has a bug that does not separate namespace…
(name is not used…)

Hello, thanks for the quick reply.

We are using nodejs + discord.js so we used @kvs/env

azu added a commit that referenced this issue Feb 16, 2022
@azu
Copy link
Owner

azu commented Feb 16, 2022

Thanks to confirm.
I understant it is bug.

#21 will fix it, but it will includes breaking change.
so, I plan to publish it as major update. (probably, weekend)

@berkbuzcu
Copy link
Author

We walked around this but it's kind of a problem if it fetches every item at every call.

Thanks for the fix!

@azu azu closed this as completed in #21 Feb 19, 2022
@azu
Copy link
Owner

azu commented Feb 19, 2022

This issue is fixed in https://github.com/azu/kvs/releases/tag/v2.0.0

Thanks for report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug or Bug fixes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants