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

Jesse's database depends on the CWD => changing it breaks jesse #50

Closed
wk8 opened this issue May 18, 2017 · 5 comments
Closed

Jesse's database depends on the CWD => changing it breaks jesse #50

wk8 opened this issue May 18, 2017 · 5 comments
Labels

Comments

@wk8
Copy link
Contributor

wk8 commented May 18, 2017

It seems that starting from 820d0e9#diff-06b055de1c1c5e19892ec0ef0a7d057f, jesse stores schemas in its database using canonical keys, which, unfortunately, depend on the current working directory (see

"file://" ++ filename:join(raw_canonical_path(filename:absname(Path))).
and https://github.com/erlang/otp/blob/05dce0f330c83278cb134c7235a5353ce4116307/lib/stdlib/src/filename.erl#L73).

In particular, that means this:

1> jesse:add_schema(key, {[]}).       
ok
2> jesse:validate(key, {[]}).
{ok,{[]}}
3> file:set_cwd("/").        
ok
4> jesse:validate(key, {[]}).
{error,{database_error,"file:///key",schema_not_found}}

which I think should be regarded as a bug?

Two questions here:

  1. do you guys see this behaviour as a bug too? (if not, why?)
  2. I'd be very happy to offer a patch, but since I'm not sure I get the rationale behind 820d0e9, I'd very much like some advice on how to go about this.

Thanks! :)

@wk8 wk8 changed the title Jesse database depends on the CWD => changing it breaks jesse Jesse's database depends on the CWD => changing it breaks jesse May 18, 2017
@andreineculau
Copy link
Member

the canonical keys refactoring are related to #13 (comment)

as for the behaviour, it is correct (though I agree the function docs should be updated).
if you look at the implementation, both the 1st 2nd and 3rd expression in your erlang shell translates key to file://$(pwd)/key

because jesse didn't support $ref in the beginning, it had added jesse:add_schema, except it shouldn't have been so liberal with the key type, and in turn it created the impression that they key is absolute even as a non-uri, when in fact it is not.

Makes sense? If so, please close the issue. If not, fire away. Thanks!

@andreineculau
Copy link
Member

I think better words would be

  • use an absolute key e.g. jesse:add_schema("file:///key", {[]})
  • or give your schema an id e.g.
2> jesse:add_schema(key, {[{<<"id">>,<<"key">>}]}).
ok
3> jesse:validate(key, {[]}).
{ok,{[]}}
4> file:set_cwd("/").
ok
5>
5> jesse:validate("key", {[]}).
{error,{database_error,"file:///key",schema_not_found}}

Now this is a bug :) because the 5th expression (note the quotes) should have succeeded. Because there is a schema with an id "key". There's a bug in jesse_database:load/1 which I have just fixed.

@wk8
Copy link
Contributor Author

wk8 commented May 22, 2017

@andreineculau : not sure I get why has to call jesse:add_schema/2 with an atom and then jesse:validate/2 with a string instead to mean the same schema? Seems rather counter-intuitive?

Other than that, yes, that solves the issue, thanks :)

@andreineculau
Copy link
Member

the difference is not so much in atom vs list, but in SourceKey vs Id, that is lookup schema via how it was loaded, or via its id value (property), and the id can only be a string.

As per the spec, all keys should be "strings", but in reality it accepts anything that can converted to a string (if memory holds true it's for keeping some backwards compatibility).
I have introduced a guard which clarifies that, allowing only lists (people will have to convert atoms themselves). Thanks for bringing this up.

@andreineculau
Copy link
Member

I believe #59 would also cover this case. Closing.

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

No branches or pull requests

2 participants