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

New unit for NetHTTP #33

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

New unit for NetHTTP #33

wants to merge 20 commits into from

Conversation

p-samuel
Copy link
Member

@p-samuel p-samuel commented Jan 15, 2023

  • A new unit was created packing the NetHTTP routines. This unit will be used as an alternative to the already existing Indy unit and will be instanciated in the API factory unit. The developer will be able to choose which library he wants to utilize to make the ntfy HTTP comunication by a compiler directive called NTFY_HTTP_INDY.
Notify.Api.Factory

function TNotifyApiFactory.Api: INotifyApi;
begin
  {$IFDEF NTFY_HTTP_INDY}
  Result := TNotifyApiIndy.New
  {$ELSE}
  Result := TNotifyApiNetHTTP.New;
  {$IFEND}
end;

Without this compiler directive, the library will utilize NetHTTP by default. The developer can alter accessing Project > Options > Building > Delphi Compiler > Conditional Defines and adding NTFY_HTTP_INDY on the conditional defines list.

  • Sample directory was renamed to samples, so that the dependency manager ignores this folder when installing, avoiding the units inside this directory to be added on the project's search path.

  • A new example was included demonstrating how utilizing this library on windows services.

  • Two warnings were also removed at Notify.Api.Indy unit. They were caused by implicit cast operation when a string variable was assigned to a UTF8String variable.

LStrings := SplitString(UTF8ToString(LEventString) , #$A);

  for LString in LStrings do
    NxHorizon.Instance.Post<TNotifySubscriptionEvent>(Utf8String(LString));
  • TNotifyResponseData was renamed to TNotifyResponseDTO.

  • In case the developer opts to utilize Indy components, the openssl libraries will be loaded according to the operational sistem of the client. It has not yet been tested in some operational systems.

@p-samuel p-samuel added the enhancement New feature or request label Jan 15, 2023
@p-samuel p-samuel self-assigned this Jan 15, 2023

function TNotifyApiFactory.Api: INotifyApi;
begin
{$IFDEF INDY}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs documenting in the README.md. It's effectively a part of your public interface "how to switch between Indy/NetHTTP".

Also, consider renaming this compiler define to NTFY_INDY. Consider, with such a generic define based on the name of a very popular network component set, what would happen if your library was included in other large projects that may have libraries with the same defines for other purposes? You may even consider having a more descriptive name NTFY_DO_A_THING_WITH_INDY so it becomes self-documenting when people include it in their projects.

Copy link
Member Author

@p-samuel p-samuel Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking out for improvement. I'll make the necessary changes. I think NTFY_INDY is more descriptive name as you mentioned as it avoids clashing with other name that a large library might have. I'll also provide documentation of that in the README.md file. I'll update the files with the above review before merging.

Copy link
Member Author

@p-samuel p-samuel Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhatherall, your review has been submitted. Thanks for your contribution.

- Updated conditional defines for switching between Indy and NetHTTP
- Updated readme documentation
- FreeAndNil removed from Api classes.
@p-samuel
Copy link
Member Author

p-samuel commented Jan 17, 2023

♻ Refactoring

  • Updated conditional defines for switching between Indy and NetHTTP. (NTFY_HTTP_INDY)
  • Updated readme documentation
  • FreeAndNil removed from Api classes.

@p-samuel
Copy link
Member Author

p-samuel commented Jan 19, 2023

♻ NetHTTP Compatibility issues.

  • There has been noticed some issues when installing this library on Delphi releases lower than 10.4 Sydney.
  • I'll proceed to refactor two points where the NetHTTP class that breaks on these release.
function TNotifyApiNetHTTP.ClearHeaders: INotifyApi;
begin
  Result := Self;
  FNetHTTPRequest.CustHeaders.Clear; //does not exists in Delphi < 10.4 Sydney
end;
procedure TSSEThread.AbortStream;
begin
  FNetHTTPResquest.Cancel; //Does not exists in Delphi < 10.4 Sydney
end;

Unfortunately, I could not find a solution to cancel the long polling connection with NetHTTP for mantain retro-compatibility. NetHTTP class was simply incomplete from product version 26 to 27 and that was a shock to me. If you are using a product version lower or equal than 26 (10.3) you'll not be able to unsubscribe from a topic unless you close your application or service.
@p-samuel
Copy link
Member Author

p-samuel commented Mar 27, 2023

From product version 26 to 27, Embarcadero completed the NetHTTP class to perform an abort operation on current requests in transaction. Although many years has passed since the first release of NetHTTP, this class was incomplete and lacking these basic methods even up until the release 26 - something that shocked me when I realized. This is a huge drawback if you are using Delphi 10.3 and lower. It seems they didn't have in mind that a client could possibly perform a long polling operation.

Because of that, if we are using a product version lower or equal to 26 with NetHTTP, the library will be unable to unsubscribe from a topic unless we close and restart the application or service. Yes, huge drawback for retro-compatibility. Otherwise, everything remains as it before. From Delphi 10.4+, nothing has been changed except this new NetHTTP class.

Copy link
Member Author

@p-samuel p-samuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready. I'm just working on a small example for Android subscriber.

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

Successfully merging this pull request may close these issues.

None yet

2 participants