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

Debugging Help in error logs #80

Closed
shaheenery opened this issue Dec 3, 2020 · 5 comments
Closed

Debugging Help in error logs #80

shaheenery opened this issue Dec 3, 2020 · 5 comments

Comments

@shaheenery
Copy link

I tried digging in a bit to see if I could add this, but I couldn't figure it out. This test encapsulates the behavior that I'm looking for.

A problem that I have is that if the XML document is not the shape that I am expecting e.g. missing a non-optional field, my process crashes. I log the error and stack trace and throw the correlated message on my rabbitMQ error queue. But, there is no way to quickly say what the problem is.

The stack trace doesn't say the line were the missing element caused the problem, typically: [error] ** (Protocol.UndefinedError) protocol Enumerable not implemented for nil of type Atom.

If anyone could help point me in the right direction, that would be great, too.

  test "gives useful info with xpath with route that doesn't exist", %{simple: simple} do
    fun = fn -> 
      xpath(simple, ~x"//ListBucketResult",
        name: ~x"./Name/text()"s,
        is_truncated: ~x"./IsTruncated/text()"s,
        owner: [
            ~x"./Owner",
            id: ~x"./ID/text()"s]) == nil
    end

    expected = "ElementNotFound: <ListBucketResult> is not found in <html>"

    assert capture_log(fun) =~ expected
  end
@devstopfix
Copy link

The protocol error indicates that you have written a valid set of nested Elixir keywords in your xpath, but it does not conform to a valid nested expression at runtime

Maybe use xmap instead of xpath, and if you return a list of buckets with ~x"//ListBucketResult"l (notice the trailing L) then you can check for the empty case as you get an empty list

Something like

xmap(simple, buckets: [
     ~x"//ListBucketResult"l,
        name: ~x"./Name/text()"s,
        is_truncated: ~x"./IsTruncated/text()"s
])

would give you a possibly empty list of maps with keys name and is_truncated

@shaheenery
Copy link
Author

@devstopfix , I appreciate the time you took to offer your suggestion, thank you. That would definitely solve the problem of the parsing error.

With the documents that I am working with, there can be problems with the XML because there is another team generating it. I think I would have to use this technique for every section of the XML. I would be treating every path as a list, then check if the result is an empty list or hopefully a list of length 1.

I would have this logic scattered throughout the code for each path I'm interested in, 50-100. I could write a more general function, but I would still be calling it from these 50-100 places. That's why I thought it would be good to put it in the library itself.

The way I'm thinking about it, if you are expecting a path to exist. (You don't mark it optional with the ~x""o, you are probably still writing your parser, debugging your project, or have come across an invalid document. (invalid in the sense that your parsing logic is a sort of schema validator) In any of these cases, knowing the discrepancy between the parser and the xml is super helpful.

I like your suggestion better than my own workaround idea. For each of the nested xpath calls, put the parsing of the nested portion in its own method, so at least you could have line numbers. But then, the nested SweetXML sigils aren't really representative of the shape of the XML document, which is one of the things that I think makes the library very helpful.

I'll have to give it another shot when I have some time.

Thanks again

@Shakadak
Copy link
Member

Shakadak commented Mar 3, 2021

Hi,

I am interested in having this kind of behavior too, but our backward compatibility requirement is a barrier to it. This feature could be brought to a new module, but if we go this way, I would like to formalize the behavior of the library in order to remove as much as possible the incoherent or counter-instinctive behaviors. But this is not the current focus at work, so I don't expect we'll be able to work on this in the next few months

Regarding alternatives, you could try to write a wrapper around xpath and xmap, using transform_by and the fields of the SweetXpath struct, something of the sort:

def xpath!(input, spec, subspec) do
  spec = raise_on_nil(spec)
  subspec = spec_map(subspec, &raise_on_nil/1)
  xpath(input, spec, subspec)
end

def raise_on_nil(spec) do
  transform_by(spec, fn x -> case spec.transform_fun.(x) do
    nil -> raise("#{__MODULE__}.xpath! -- #{spec.path} not found")
    x -> x
  end end)
end

def spec_map(%SweetXpath{} = spec, updater),              do: updater.(spec)
def spec_map({name, %SweetXpath{} = spec}, updater),      do: {name, updater.(spec)}
def spec_map(specs, updater) when is_list(specs),         do: Enum.map(specs, fn spec -> spec_map(spec, updater) end)
def spec_map({name, specs}, updater) when is_list(specs), do: {name, Enum.map(specs, fn spec -> spec_map(spec, updater) end)}

I think it can be made more advanced by threading the paths inside the nesting to make the raised message clearer

@shaheenery
Copy link
Author

shaheenery commented Mar 3, 2021

I am interested in having this kind of behavior too, but our backward compatibility requirement is a barrier to it.

Oh man, that's a bummer. I don't want to break anyone. I thought adding into to the error message might be safe, but I'll have to learn more.

Regarding alternatives, ...

Thank you so much, this is a really nice suggestion.

@Shakadak
Copy link
Member

Shakadak commented Mar 4, 2021

Hey, so I thought a bit back on it, and we could add a modifier to the sigil, for example a for assertively, that would raise in case :xmerl_xpath.string finds nothing, but there are still a few problems:

  • the modifier o is its counterpart, and as you said, it behaves in an unexpected manner, still that could be a viable addition so I'll discuss about it with the others at work
  • you will still need to add it everywhere you use the ~x sigil or the SweetXpath struct, that's the opposite of what you were asking for
  • in order to have an error that is as meaningful as possible, we need to raise the internal complexity of the library, that's our job to make it work, but this also means this issue will get pushed back in term of priority as we need to address other issues that would be more complexe to resolve if worked on afterward

I'm closing this issue, and I'll link to it when I'll start the effort on formalization

Still, feel free to ask more questions if needed

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

No branches or pull requests

3 participants