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

HOXT: Fix getIQChildElementBuilder in AbstractHttpOverXmpp.java + Update documentation #48

Closed
wants to merge 2 commits into from

Conversation

Tibo-lg
Copy link
Contributor

@Tibo-lg Tibo-lg commented Jun 22, 2015

  • NullPointerException was raised when not setting any header or data in an HttpOverXmppReq or Resp. Fixed by using optAppend instead of append.
  • The documentation used a Set for storing the head bu this is not accepted by HeadersExtension constructor.
  • Error occurs if no version in set in a Req or Res. This is not fixed as I am not sure what should be the correct behavior. Two possibilities:
    • Mandatory to specify version, then an exception should probably be raised when it is not done.
    • Version number is set by default to "1.1" if not provided. In that case should the Host header be set by default as well?

PS: Sorry I messed up the previous request trying to squash the commits.

@Flowdalic
Copy link
Member

This is not fixed as I am not sure what should be the correct behavior.

That's a design flaw in Smack's HOXT API: Its classes where designed as Java Beans with getters and setters. Whereas it should use a builder and create immutable instances (Which is what new Smack code should do). The build() operation should fail if mandatory fields are not set to correct values (like 'version' in this case). Having a default value of "1.1" sounds ok in this case.

PS: Sorry I messed up the previous request trying to squash the commits.

FYI, there was no need to create a new PR. You could have simply update the existing one by doing a force-push to its source branch.

@Flowdalic
Copy link
Member

Applied as e20c170 and 3ff1ab6

@Flowdalic Flowdalic closed this Jun 22, 2015
@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Jun 22, 2015

That's a design flaw in Smack's HOXT API: Its classes where designed as Java Beans with getters and setters. Whereas it should use a builder and create immutable instances (Which is what new Smack code should do). The build() operation should fail if mandatory fields are not set to correct values (like 'version' in this case). Having a default value of "1.1" sounds ok in this case.

I will try to update it but it may take some time.

FYI, there was no need to create a new PR. You could have simply update the existing one by doing a force-push to its source branch.

Thanks for the tips!

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