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

/tags should give a list of tags #30

Closed
hannesm opened this Issue Apr 22, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@hannesm
Copy link
Collaborator

hannesm commented Apr 22, 2016

currently it returns a not found

@voila

This comment has been minimized.

Copy link
Contributor

voila commented Apr 27, 2016

How would you get the list of tags ? Do you propose to walk KeyHashtbl and for each article to collect its tags ? Or should we get the tags directly from the Store module ?

@hannesm

This comment has been minimized.

Copy link
Collaborator

hannesm commented Apr 27, 2016

I'd naively walk the KeyHashtbl.. if it turns out to be a performance bottleneck, caching can be done later

@Engil

This comment has been minimized.

Copy link
Owner

Engil commented Apr 27, 2016

I guess walking through KeyHashtbl and building a Set of all tags is enough for now, yeah. We'll cache it one day if needed.
No idea on where to put the /tags link, though

@dbuenzli

This comment has been minimized.

Copy link

dbuenzli commented Apr 27, 2016

Is this done statically or dynamically in the unikernel ?

@Engil

This comment has been minimized.

Copy link
Owner

Engil commented Apr 27, 2016

dynamically in the unikernel
Everytime new content is pushed to the repository, after pulling it, Canopy walks through the repository (could be better) and cache parsed articles in a hashtable.
Tags are available in each article records in this hashtable

@dbuenzli

This comment has been minimized.

Copy link

dbuenzli commented Apr 27, 2016

Then I'd rather use a Map.Make than a hashtable. This a fullproof way of preventing certain class of dos attacks on hashtables (e.g. if the contents can be user-contributed). In general I don't understand people who start with hashtables first... always start with maps and only consider replacing it with a hash table if that turns out to be a problem.

@Engil

This comment has been minimized.

Copy link
Owner

Engil commented Apr 27, 2016

Got this, even though I'm not too sure about how user-contributed the content really is.
Contributing content imply having a write and a push access to a git repository (implying the push access is sufficient to trigger the push hook mechanism that the user can define).
I guess we can move to a Map with no problem, but how security is handled in Canopy is still a bit blurry…

@dbuenzli

This comment has been minimized.

Copy link

dbuenzli commented Apr 27, 2016

It may not even be possible to exploit this. It's just an algorithmic mindset: by using a Map you just evacuate this possibility from the software altogether.

Also as the software evolves that bit of code may end up being (re-)used in another context or c&p in other software without much thought, but in a use case where the problem may become relevant.

In any case if you stick to hashtables it would be better to create the custom hashtable with Hashtbl.MakeSeeded and redefine the create function in KeyHasthbl to automatically create hashtables with ~random:true.

@Engil

This comment has been minimized.

Copy link
Owner

Engil commented Apr 27, 2016

I agree, I will then move to Map.Make, I don't see any problem in doing so, and @hannesm seems to agree too.
Feedback greatly appreciated, thank you :-)

voila pushed a commit to voila/Canopy that referenced this issue Apr 30, 2016

voila pushed a commit to voila/Canopy that referenced this issue Apr 30, 2016

voila pushed a commit to voila/Canopy that referenced this issue May 2, 2016

Engil added a commit that referenced this issue May 2, 2016

Merge pull request #39 from voila/tags
issue #30 /tags should give a list of tags

@Engil Engil closed this May 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment