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

With 0.3.1 all ldap3 tests fail #52

Closed
cannatag opened this issue Jul 26, 2017 · 7 comments
Closed

With 0.3.1 all ldap3 tests fail #52

cannatag opened this issue Jul 26, 2017 · 7 comments

Comments

@cannatag
Copy link

cannatag commented Jul 26, 2017

Hi Ilya, with version 0.3.1 I get the following error on every test:

...
  File "C:\Python\OpenSource\ldap3\ldap3\operation\bind.py", line 124, in bind_response_to_dict
    'saslCreds': bytes(response['serverSaslCreds']) if response['serverSaslCreds'] is not None else None}
  File "C:\Python\Python36\lib\site-packages\pyasn1\type\univ.py", line 965, in __bytes__
    return bytes(self._value)
TypeError: cannot convert 'NoValue' object to bytes
  File "C:\Python\Python36\lib\site-packages\pyasn1\type\univ.py", line 965, in __bytes__
    return bytes(self._value)
TypeError: cannot convert 'NoValue' object to bytes

All tests were ok with version 0.2.3.

@etingof
Copy link
Owner

etingof commented Jul 26, 2017

Hi Giovanni!

I think this is the same regression as we discussed lately. Can you please add the .hasValue() check in addition to is None you have like this:

    'saslCreds': bytes(response['serverSaslCreds']) if response['serverSaslCreds'] is not None and response['serverSaslCreds'].hasValue() else None}

I'm here to fix whatever regression the latest release may have caused. %-\

@cannatag
Copy link
Author

I'm adding a lot of if request['xxx'] is not None and request['xxx'].hasValue() else None to the ldap3 code. I think that there should be a better way to do this. With the previous pyasn1 versions the uninitialized value were all None. There is a way to have the same behaviour?

@etingof
Copy link
Owner

etingof commented Jul 27, 2017

Well, I'm not sure how to handle this in a backward compatible way... The ultimate goal is to make the whole API more "automatic" and compact. For example, imagine a nested structure like this:

    LDAPMessage ::= SEQUENCE {
             messageID       MessageID,
             protocolOp      CHOICE {
                  bindRequest           BindRequest,
                  ...
                  intermediateResponse  IntermediateResponse },
             controls       [0] Controls OPTIONAL }

    BindRequest ::= [APPLICATION 0] SEQUENCE {
             version                 INTEGER (1 ..  127),
             name                    LDAPDN,
             authentication          AuthenticationChoice }

Now you want to initialize it. With previous pyasn1 API you could only do that in multiple steps, from outer to inner elements:

    ldap = LDAPMessage()
    ldap.setComponentByName('protocolOp')  # this instantiates CHOICE
    ldap['protocolOp'].setComponentByName('bindRequest')  # this instantiates BindRequest
    ldap['protocolOp']['bindRequest'].setComponentByName('version', 1)  # this instantiates INTEGER

The new API is more compact and intuitive:

    ldap = LDAPMessage()
    ldap['protocolOp']['bindRequest']['version'] = 1

The compact version can only work if ldap['protocolOp'] returns CHOICE instance, not None for uninitialized field.

WDIT? Your advise is much appreciated!

BTW, if you make pyasn1 >= 0.3.1 a requirement, you could omit the request['xxx'] is not None part and leave just request['xxx'] if request['xxx'].isValue else None . The is not None part is only required if you want the code to work with both pyasn1 API flavours.

@cannatag
Copy link
Author

cannatag commented Aug 2, 2017

Thanks Ilya, I've been able to fix all errors in ldap3 tests. Now my codes works with pyasn1 froom version 0.1.8 up to 0.3.1. Prior to 0.1.8 the hasValue() method is absent.

@etingof
Copy link
Owner

etingof commented Aug 2, 2017

Thank you, Giovanni! And sorry for the trouble. ;)

I'd also appreciate if you run your test with the current pyasn1 master (which is about to be released as 0.3.2). I do not expect any breakage there though.

Prior to 0.1.8 the hasValue() method is absent.

I believe 0.1.8 is so much historical now days. I can potentially propose a compatibility measure to make ldap3 running with older pysn1. Let me know if you need that.

@cannatag
Copy link
Author

cannatag commented Aug 3, 2017

I've tested the 0.3.2 branch, everything is ok, all tests pass!

@etingof
Copy link
Owner

etingof commented Aug 3, 2017

Great, thank you!

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

2 participants